net/http: add requests count metric with status code label #771
Conversation
nabokihms
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MrAlias,
paivagustavo and
XSAM
as code owners
|
|
Signed-off-by: m.nabokikh <[email protected]>
Signed-off-by: m.nabokikh <[email protected]>
nabokihms
force-pushed
the
net-http-request-count-status
branch
from
a7349b5 to
f4d780a
Compare
|
I rebased the PR into the main branch and added a changelog entry. If it is possible, I would like to move this forward to merge. Maybe I have missed something from the contributing guide? Thanks in advance for any feedback. |
There was a problem hiding this comment.
Thanks for the contribution and I apologize for taking so long to respond to it. We have been deferring the addition of new features in contrib instrumentation while we worked through stabilizing the tracing API and SDK and separating it from the metrics API and SDK.
Now that we have done that, we will need to review the instrumentation in contrib with a view toward making it stable (v1.0.0 with a stable public API and no unstable dependencies). Unfortunately, I believe that this will require extracting the metrics capability from this instrumentation. I'm not sure at this time what form a new HTTP metrics implementation will take or what metrics it will provide.
I'm of two minds with regard to adding to the existing metrics instrumentation. On the one hand I don't want to add to something that will be removed shortly, while on the other the fact that it will be removed shortly means it probably doesn't hurt to add it. I will conditionally approve this PR and the other @open-telemetry/go-approvers can weigh in with their thoughts.
| ### Added | ||
|
|
||
| - Add total requests counter by response status code for the `net/http` instrumentation. (#771) | ||
|
|
There was a problem hiding this comment.
This would need to be moved up into the "Unreleased" section above.
| batchName := batch.Measurements[0].Instrument.Descriptor().Name() | ||
| if batchName == RequestCount { | ||
| statusCodeBatch = batch | ||
| continue |
There was a problem hiding this comment.
is this continue needed?
If I am not missing anything then I think that we could assertMetricAttributes for RequestCount metric as well
There was a problem hiding this comment.
Thanks, I will check it. Maybe I missed something.
@Aneurysm9 can you elaborate why that is necessary and how things would look like from your perspective after that? I'm trying to understand how things would work, so we can build something around it while the reference implementation is finished. |
|
unsolicited reply regards to why this is required: it's pretty natural for me to think about the RED method when it comes to monitoring services. While rate and duration are covered by otelhttp, error is not, and HTTP status code is a great way to observe if and why the service doesn't fulfill the request. BTW it's also covered in the open-telemetry HTTP metrics spec, among many others, though it's not quite clear to me the relation between this one and the spec, and the spec only mentions gauges, but no counters, which is quite interesting. |
|
Any update on this one? |
|
I had no chance to work on this one, bit I am still willing to add this feature. @Aneurysm9 I will rebase this PR if you don't mind. |
I would hold off until next week. We just released a new metrics API and will be updating the instrumentation in this repo shortly to use it. |
|
#1977 is the PR to watch for API changes, or wait for the subsequent release. |
Closes #711
This PR adds a counter for the total amount of requests with the http_status_code label. It will help us to be aware of the ratio of errors.
P.S. I decided to avoid adding status code to other metrics because it looks excessive. Instead, counting the number of requests seems more sensible.