-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[httpcheckreceiver] new receiver #14191
Conversation
355cdc7
to
3b688b9
Compare
type: string | ||
|
||
metrics: | ||
httpcheck.success: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth considering any other representations of this metric? One thought is that if this were httpcheck.status
where the value is the status code, then you wouldn't have to interpret what success means. Success, failure, etc could be determined on the query side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, the representation could be the status code itself (which is currently an attribute). I don't have a strong preference one way or the other. The success/failure metric is similar to probe_success
emitted by https://github.com/prometheus/blackbox_exporter. It does mean that this receiver will need to have a way for users to configure success/failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do you expect users to check the method attribute? e.g. probe both GET and POST in the same service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldn't expect someone to want to hit the same endpoint with different methods. i would expect consumers of the telemetry to want to be able to filter responses by the method though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make a lot of sense to keep the status code as an attribute, but also to change the meaning of the value from "success" to "a single observation of this status code".
You would expect subsequent observations to look like this:
{ "time": "t0", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
{ "time": "t1", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
{ "time": "t2", "url": "/foo", "method": "GET", "code": 404, "value": 1 } // outage
{ "time": "t3", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
See my other comment for more on why I think this model is useful for summarization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed your other comment, and I think this would be a great way to represent this sort of information! A unit seems like the best place I can think of for putting that information, and it makes sense in contexts other than Prometheus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could take a similar approach for info metrics as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Symbolic approval, this looks good! Also interested to help improve this with a few potential additions ;) |
e12c1b1
to
68920ae
Compare
type: string | ||
|
||
metrics: | ||
httpcheck.success: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make a lot of sense to keep the status code as an attribute, but also to change the meaning of the value from "success" to "a single observation of this status code".
You would expect subsequent observations to look like this:
{ "time": "t0", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
{ "time": "t1", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
{ "time": "t2", "url": "/foo", "method": "GET", "code": 404, "value": 1 } // outage
{ "time": "t3", "url": "/foo", "method": "GET", "code": 200, "value": 1 }
See my other comment for more on why I think this model is useful for summarization.
type: string | ||
|
||
metrics: | ||
httpcheck.success: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8876eb4
to
847c930
Compare
Thanks to everyone for all the input. I've updated I've also added another metric to allow users to count the number of errors that occurred. Previously, i was using the status code @mwear @djaglowski @jmacd PTAL |
3a9177c
to
7ad3976
Compare
7586751
to
e46a175
Compare
The HTTP Check Receiver can be used for synthethic checks against HTTP endpoints. This receiver will make a request to the specified `endpoint` using the configured `method`. Fixes #10607
e46a175
to
08cb225
Compare
The HTTP Check Receiver can be used for synthethic checks against HTTP endpoints. This receiver will make a request to the specified `endpoint` using the configured `method`. Fixes open-telemetry#10607
The HTTP Check Receiver can be used for synthethic checks against HTTP endpoints. This receiver will make a request to the specified
endpoint
using the configuredmethod
.Here's an example configuration:
The metrics being emitted:
Fixes #10607