-
Notifications
You must be signed in to change notification settings - Fork 576
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
Add metrics for otelhttp client #427
Conversation
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.
The http metrics we implement need to conform to the OpenTelemetry metric semantic conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/semantic_conventions/http-metrics.md
Thanks for the feedback, I will change it later. |
@MrAlias I have updated this PR according to your document, can you review it again? |
track.end() | ||
} else { | ||
track.body = resp.Body | ||
resp.Body = wrappedBodyIO(track, resp.Body) |
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.
Reading of the response body should not be included in the measurement of a requests duration. Right?
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.
The request will be counted as ended after we finish reading all the data from the response body, won't it?
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 is probably utility in measurements of both time to first byte and time to the end of the body being read, though the latter is potentially less a measure of the client and more of whatever is processing the response data.
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.
This only wrap and returns reader interface rather than actual read and parsing actual data itself. The actual parsing time could be useful, but should not be included in this package, dont you think ?
bb3a22d
to
8dd6cef
Compare
39e4b87
to
b166165
Compare
func NewTransport(base http.RoundTripper, opts ...Option) *Transport { | ||
t := Transport{ | ||
rt: base, | ||
func NewTransport(base http.RoundTripper, opts ...Option) http.RoundTripper { |
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.
Since this is the primary mechanism by which I expect otelhttp.Transport
instances to be created and this leaves no configuration option for enabling/disabling metric collection, does it make more sense to integrate the metrics collection directly in the Transport.RoundTrip
method? This would also be more in line with the approach taken by the otelhttp.Handler
.
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 prefer to split up the implementation of metrics and tracing parts for ease of maintenance.
// Perform request.; | ||
resp, err := trans.base.RoundTrip(req) | ||
if err != nil { | ||
tracker.labels = append(labels, semconv.HTTPAttributesFromHTTPStatusCode(http.StatusInternalServerError)...) |
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.
This should include the response status not http.StatusInternalServerError
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.
In case of HTTP connection error occurred, should we recorded nothing or 500 as the HTTP status code. What's your opinion?
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Add some metrics for the otelhttp transport for http client net/http
Includes those metrics:
http_client_duration
Tested sample
Referencing: https://github.com/census-instrumentation/opencensus-go/blob/master/plugin/ochttp/stats.go