Stale markers #73
Stale markers #73
Conversation
codebien
force-pushed
the
stale-marker
branch
2 times, most recently
from
12a0f06
to
3f60417
Compare
codebien
force-pushed
the
stats-time-seconds
branch
from
e3acfb5
to
fddb22f
Compare
codebien
force-pushed
the
stale-marker
branch
3 times, most recently
from
d1dc351
to
023b1f2
Compare
codebien
force-pushed
the
stale-marker
branch
5 times, most recently
from
53068f2
to
7946600
Compare
| // a time series as stale. | ||
| // | ||
| // https://pkg.go.dev/github.com/prometheus/prometheus/pkg/value#pkg-constants | ||
| staleNaN = math.Float64frombits(0x7ff0000000000002) |
There was a problem hiding this comment.
Would it make sense to make this particular value a constant?
There was a problem hiding this comment.
Can't we just use the StaleNaN constant from the prometheus package directly?
There was a problem hiding this comment.
I'm looking into this but I'm not sure that the alternative is really better because the const would be a uint64 and it would be accepted if assigned to a float64. So if we forget to convert with math.Float64frombits( then we introduce a bug because the compiler doesn't complain. We should only rely on unit tests. @oleiade WDYT?
There was a problem hiding this comment.
Can't we just use the StaleNaN constant from the prometheus package directly?
@imiric It would require including the Prometheus project as a direct dependency, but it has a lot of dependencies.
| // a time series as stale. | ||
| // | ||
| // https://pkg.go.dev/github.com/prometheus/prometheus/pkg/value#pkg-constants | ||
| staleNaN = math.Float64frombits(0x7ff0000000000002) |
There was a problem hiding this comment.
Can't we just use the StaleNaN constant from the prometheus package directly?
|
|
||
| // TODO: warn if it the request is taking too long (5 seconds?, the same timeout set for flushing) | ||
|
|
||
| err := o.client.Store(context.Background(), staleMarkers) |
There was a problem hiding this comment.
Hhmm shouldn't this use some extension-wide context? Preferably one passed from k6? I now see that output.Params doesn't have a context. Maybe it should?
There was a problem hiding this comment.
IMHO, this falls down to grafana/k6#2430 (comment). We should consider prioritizing it during the next cycle.
For this version, I think the unique way we have is to implement the TODO directly now logging in case of a long Stop execution. WDYT?
There was a problem hiding this comment.
For this version, I think the unique way we have is to implement the TODO directly now logging in case of a long Stop execution. WDYT?
Thinking better on this, the current client is already initialized with the Timeout value from the configuration, so the request timed out when the timeout interval is passed. It should be enough for warning in the case of a long-time request and to alarm for a required investigation into eventual bad performance between k6 and the Remote write endpoint.
There was a problem hiding this comment.
I'm not sure what the best approach would be. Just thought it would probably make more sense to use an existing context, instead of using context.Background() everywhere.
Intuitively, it makes sense to me for the extension context to be based on one received by k6, so that the extension code can cancel any operations in progress if the k6 context is cancelled (due to a signal, test abort, etc.). But if you think that the timeout value from the configuration is enough, then we could go with that instead. I think it can be done in a separate PR, though.
There was a problem hiding this comment.
Intuitively, it makes sense to me for the extension context to be based on one received by k6, so that the extension code can cancel any operations in progress if the k6 context is cancelled (due to a signal, test abort, etc.).
Just to be clear, I agree here, and this is something we want to add in the future, this is why the context is passed to the wait function in the Output refactor proposal (wait func(context.Context) error, err error).
Unfortunately, I think we can't do it now, because it would require some changes to k6 when the code is almost frozen.
|
|
||
| // TODO: warn if it the request is taking too long (5 seconds?, the same timeout set for flushing) | ||
|
|
||
| err := o.client.Store(context.Background(), staleMarkers) |
There was a problem hiding this comment.
I'm not sure what the best approach would be. Just thought it would probably make more sense to use an existing context, instead of using context.Background() everywhere.
Intuitively, it makes sense to me for the extension context to be based on one received by k6, so that the extension code can cancel any operations in progress if the k6 context is cancelled (due to a signal, test abort, etc.). But if you think that the timeout value from the configuration is enough, then we could go with that instead. I think it can be done in a separate PR, though.
codebien
force-pushed
the
stale-marker
branch
from
a55f02d
to
235be21
Compare
Added a Stale marker at the end of the test for all the seen time series.
Closes #68 #37