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

url.scheme is present on HTTP client metrics but not HTTP client spans #450

Closed
trask opened this issue Oct 25, 2023 · 5 comments · Fixed by #459
Closed

url.scheme is present on HTTP client metrics but not HTTP client spans #450

trask opened this issue Oct 25, 2023 · 5 comments · Fixed by #459
Assignees

Comments

@trask
Copy link
Member

trask commented Oct 25, 2023

url.scheme was recently added to HTTP client metrics in #357, but we didn't add it to HTTP client spans.

this is problematic for a couple of use cases:

  • span-to-metric pipelines
  • Java (and other?) instrumentation APIs which unify span and metric creation, passing only a single set of attributes and using metric advisory parameters (hints) to reduce down to the set of metric attributes
@lmolkova
Copy link
Contributor

The motivation to not have a scheme on client spans was duplication (we already have scheme in the URL).

Span-to-metrics pipelines can extract scheme from the URL - this way a majority(?) of users who don't use such pipelines would not get duplicated data.

Scheme also is not a single parameter that's different between metrics and traces (e.g. URL only appears on traces).

I'd prefer not to add a scheme to client spans and we can always revisit it in the future since it's a tracing attribute.

@trask
Copy link
Member Author

trask commented Oct 27, 2023

I'd prefer not to add a scheme to client spans

ya, I agree with this 👍

wdyt about moving url.scheme back to opt-in on metrics?

in Java at least we may need to make it opt-in due to our design constraint around metric attributes being a subset of span attributes (until someday maybe when tracing attribute advice is spec'd out similar to metric attribute advice)

@lmolkova
Copy link
Contributor

lmolkova commented Oct 27, 2023

wdyt about moving url.scheme back to opt-in on metrics?

The motivation to add it to metrics was that we have the condition on the port "If not default (80 for http scheme, 443 for https).", but there was no scheme to tell

If we remove the scheme, we should probably make the port required on metrics.
Following the same consistency principle, we should probably then make it required for traces as well (and remove the default value).
I would not mind that. Consumers who want to stay frugal can remove default ports at export/ingestion time.

@lmolkova
Copy link
Contributor

@trask wdyt - #459?

@mateuszrzeszutek
Copy link
Member

in Java at least we may need to make it opt-in due to our design constraint around metric attributes being a subset of span attributes (until someday maybe when tracing attribute advice is spec'd out similar to metric attribute advice)

I already implemented that a while ago actually: open-telemetry/opentelemetry-java-instrumentation#9642
It is kinda hacky, but not impossible. (I still like #459 more 👍 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
4 participants