-
Notifications
You must be signed in to change notification settings - Fork 174
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
added remaining ECS fields to the useragent #452
Conversation
@jsuereth to answer your questions from my previous PR about possible multiple names/versions. |
cc @open-telemetry/sandbox-web-js-maintainers |
There are some existing definitions that might be duplicated (or at least not clear which one should be used) -
|
based on this, it's not clear to me whether the new attributes in this PR are needed |
I think the important difference here is that both A great example is NGINX (or webserver in general) access logs: As a user you would like to group the access logs / requests by device name / version, etc. Plus, we are adding attributes to the registry here and not proposing their usage in existing semantic conventions with that PR. But, rather enable logging use cases and migration of ECS-based use cases into OTel. |
could the |
The NGINX use case is a great example, and I think these attributes make more sense there than re-using the
I suppose the browser attributes could be used as a source for the user-agent name/value attributes, which is better than parsing the user-agent string. I am not sure about device name. That still seems like a duplicate to me. |
I see that this is still a draft, which means it might change/take a long time to stabilise and even after release it might take some (long) time to update existing use cases to rely on it instead of existing user-agent string.
How we could use In addition these fields are extracted from user-agent string to make later search or analysing the data easier, because the string is parsed already and most important data is extracted. Also these fields are added into registry so people could use them if needed and that gives ECS use cases a green light. Please correct me if I'm missing something here |
This was in response to @trask's question of using All that to say, I am fine with adding these attributes 👍 |
Thanks for the clarification, @martinkuba . In that case would you mind to approve the PR technically? |
I am fine with |
@martinkuba I have moved it out of this PR not to block other fields to be added. I will create a dedicated PR for this field to discuss it separately |
Unfortunately, this isn't true for gRPC, see e.g. https://stackoverflow.com/questions/60527092/how-to-specify-a-user-agent-header-for-grpc-java-client It's expected gRPC users will add their app version and gRPC will append its version. |
I have checked what will be returned for provided user-agent in example from SO post Or do you think we should expand user-agent to support all possible |
@trisch-me given this comes from ECS, how was the gRPC "problem" handled there? I'm sure probably folks using ECS ran into that or? |
I actually think both are viable solutions here. At a minimum if you currently restrict what you have to just the browser, and we have an open ticket to expand further to gRPC-like use cases, then I'd be ok merging, and we can look at the broader problem later. My ideal solution would be we define use agent name version to be "the outer most" or "most important" name/version found in the string. For |
@joaopgrassi I have talked with other folks from Elastic, and we found no such usage in our codebase. However, this doesn't necessarily mean that our customers haven't used it in that way, they might have chosen, for example, the "most dominant" or "most important" out of possible options. |
@AlexanderWert @jsuereth I have fixed conflicts. This one is good to go and I need these changes to be merged in order to have another PR opened for discussions. |
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Changes
This PR adds remaining 3 fields from ECS to the user-agent namespace
Merge requirement checklist