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

Adds extended span name. #239

Closed
wants to merge 1 commit into from
Closed

Conversation

dimaKudr
Copy link

No description provided.

@dimaKudr dimaKudr requested a review from a team February 21, 2024 17:10
Copy link

welcome bot commented Feb 21, 2024

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

Copy link

CLA Not Signed

@weslenteche
Copy link
Contributor

@dimaKudr According to the semantic convention documentation - "HTTP client spans have no http.route attribute since client-side instrumentation is not generally aware of the “route”, and therefore HTTP client span names SHOULD be {method}.".

@dimaKudr
Copy link
Author

hi @weslenteche. thank you for your response. I have checked the convention mentioned in docs and I don't find it convenient. but maybe I miss something. could you please take a look at the screenshot here? it represents a real Trace from our system. you can see there several spans to Mongo and 2 spans generated by Guzzle http client. Both Guzzle spans have the same GET method. Without the host name mentioned in those spans they will look the same and very non-informative. BTW, you can see that every Mongo span has collection and query name in its title. It looks pretty similar to what I propose for Guzzle spans.
SCR-20240223-hle

@weslenteche
Copy link
Contributor

hi @weslenteche. thank you for your response. I have checked the convention mentioned in docs and I don't find it convenient. but maybe I miss something. could you please take a look at the screenshot here? it represents a real Trace from our system. you can see there several spans to Mongo and 2 spans generated by Guzzle http client. Both Guzzle spans have the same GET method. Without the host name mentioned in those spans they will look the same and very non-informative. BTW, you can see that every Mongo span has collection and query name in its title. It looks pretty similar to what I propose for Guzzle spans. SCR-20240223-hle

Information regarding hostname, URL, for example, will be located in the span attributes. This client span has some attributes such as url.full and server.address that can help you in the analysis process.

In my opinion, by mixing this we will be opening gaps to not follow the standards of the semantic convention. But I also wanted the opinion of the @brettmc @bobstrecansky

@brettmc
Copy link
Collaborator

brettmc commented Feb 28, 2024

This looks very similar to #190 which was proposed for the same reason and rejected on the basis of spec-conformance, which @weslenteche mentioned and linked to earlier.

If you look at other implementations, I think you'll find other SIGs are following the spec and doing the same thing that we are with http span names.

re: mongo (database) spans, they're also following spec: https://opentelemetry.io/docs/specs/semconv/database/database-spans/

The span name SHOULD be set to a low cardinality value representing the statement executed on the database

IMO, if you feel really strongly about this change, then start with the spec. I couldn't find an exact matching issue, however open-telemetry/semantic-conventions#899 is similar. Perhaps using {method} {hostname} avoids the cardinality explosion concerns referred to in that issue?

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.

4 participants