Skip to content
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

Enable tracing in http.Client #896

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Enable tracing in http.Client #896

merged 4 commits into from
Mar 28, 2024

Conversation

sevein
Copy link
Contributor

@sevein sevein commented Mar 25, 2024

Register a propagator with OpenTelemetry to enable propagation of trace context over the wire 1. Update HTTP clients with otelhttp.Transport to wrap outbound requests with a span and enrich it with metrics.

This change enable us to relate spans occuring beyond an individual service, e.g. the activity "move-to-permanent-storage" executed by enduro-a3m-worker uses a HTTP client to request to the Storage Service the long-term storage of the AIP. With this commit we'll start seeing the HTTP POST operation performed by the client but also the rest of the details happening on the server side, i.e. the API handler, SQL queries, workflow, activities, etc.

image

Footnotes

  1. See the Trace Context spec (W3C Recommendation) for more details. It defines HTTP headers and a value format to standardize how context information is propagated over the wire.

@sevein sevein force-pushed the dev/http-client-tracing branch from 07e56b6 to 17e0cbc Compare March 25, 2024 10:49
@artefactual-sdps artefactual-sdps deleted a comment from codecov bot Mar 25, 2024
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 47.24%. Comparing base (f075824) to head (2829c42).
Report is 9 commits behind head on main.

❗ Current head 2829c42 differs from pull request most recent head 8e081c0. Consider uploading reports for the commit 8e081c0 to get more accurate results

Files Patch % Lines
cmd/enduro-am-worker/main.go 0.00% 7 Missing ⚠️
cmd/enduro-a3m-worker/main.go 0.00% 6 Missing ⚠️
main.go 0.00% 6 Missing ⚠️
internal/telemetry/traces.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   47.11%   47.24%   +0.13%     
==========================================
  Files          96       95       -1     
  Lines        5383     5224     -159     
==========================================
- Hits         2536     2468      -68     
+ Misses       2611     2522      -89     
+ Partials      236      234       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sevein sevein force-pushed the dev/http-client-tracing branch 3 times, most recently from 1594992 to 4c5749c Compare March 27, 2024 18:21
Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one minor change suggestion, but it's not a blocker. 👍

sevein added 4 commits March 28, 2024 16:38
Register a propagator with OpenTelemetry to enable propagation of trace context
over the wire. Update HTTP clients with `otelhttp.Transport` to wrap outbound
requests with a span and enrich it with metrics.

This change enable us to relate spans occuring beyond an individual service,
e.g. the activity "move-to-permanent-storage" executed by enduro-a3m-worker uses
a HTTP client to request to the Storage Service the long-term storage of the
AIP. With this commit we'll start seeing the `HTTP POST` operation performed by
the client but also the rest of the details happening on the server side (child
spans), e.g.: API handler, SQL queries, workflow, activities, functions...
This function could be more efficient if we introduced insert batches. This
commit should make that more apparent, e.g. will read something like:

    savePreservationTasks (703.14ms)

Which is only part of the time that took the entire activity to run, e.g. 5.33s
in my development environment.
I'm still trying to figure out what's the best way to share tracers. The SDK
encourages to operate with globals but for consistency I'm trying to share them
just like we share other values, e.g. context, logger, etc.

[skip codecov]
@sevein sevein force-pushed the dev/http-client-tracing branch from 4c5749c to 8e081c0 Compare March 28, 2024 17:02
@sevein sevein merged commit 2c6b81f into main Mar 28, 2024
11 checks passed
@sevein sevein deleted the dev/http-client-tracing branch March 28, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants