-
Notifications
You must be signed in to change notification settings - Fork 36
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
Change the URLs from HTTP GET to something useful and add the deployment environment for Elastic #32
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.
Thanks for improving the demo. I just added some small comments, please let me know your thoughts. Also, I was wondering if you would like to contribute those changes to the upstream OpenTelemetry-demo repository too, I really think in the community would value the URL attributes decoration (I am able to push those changes too if needed).
actions: | ||
- key: http.url | ||
action: extract | ||
pattern: '^(?P<short_url>https?://[^/]+(?:/[^/]+)*)(?:/(?P<url_truncated_path>[^/?]+/[^/?]+))(?:\?|/?$)' |
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.
What do you think of using the URL OTTL function in the transform processor to ensure all cases are covered? Note that this change would also provide semantic convention URL attributes (url.domand and url.path)
transform:
error_mode: ignore
trace_statements:
- context: span
statements:
- merge_maps(attributes, URL(attributes["http.url"]), "upsert")
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'll push this upstream but agree this would be useful.
trace_statements: | ||
- context: span | ||
statements: | ||
- set(name, attributes["url_truncated_path"]) |
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.
Could you provide more context about this change? I think the microservices are setting their corresponding span names, and they should follow the format {method} {format} as defined in https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name
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.
@rogercoll actually this is a good point, I was aiming for the guidance in here https://github.com/open-telemetry/opentelemetry-specification/blob/v1.33.0/specification/trace/api.md#span see
get_account | Good, and account_id=42 would make a nice Span attribute
get_account/{accountId} | Also good (using the "HTTP route")
I'll be honest I don't really understand the guidance in the semconv as it seems to contradict this. Honestly though at the moment in elastic the top transaction name becomes "HTTP GET" which is incredibly general and not useful for monitoring imo.
What do you think?
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 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.
Ah, I see. That's a really good point, thanks for raising that. I think the issue is in how the upstream services generate the root spans and which name do they set. The issue seems to only be happening for HTTP requests, GRPC services set a more human-readable name. I have just raised this issue in the upstream repository: open-telemetry#1676
Wdyt if we move this discussion upstream? Using the transform
processor in the collector might patch the issue, but I think it would be better to fix it in the root and have a concise name across all generated spans (HTTP and GRPC).
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.
Thanks for doing that @rogercoll makes perfect sense.
resource: | ||
attributes: | ||
- key: deployment.environment | ||
value: "opentelemetry-demo" |
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.
Great!
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.
Let me create a new PR with just this change for Elastic since this deployment.environment dependency is very Elastic centric.
Proposing this for merge, please let me know what you think.
Thanks
David
Changes
Please provide a brief description of the changes here.
Merge Requirements
For new features contributions please make sure you have completed the following
essential items:
CHANGELOG.md
updated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.