-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add geo fields from ECS into client and server attributes #351
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
4ae792a
Add geo fields from ECS
kaiyan-sheng 9511ad3
add changelog
kaiyan-sheng f9d0633
Merge remote-tracking branch 'upstream/main' into add_geo_fields
kaiyan-sheng 6bdb716
fix lint error
kaiyan-sheng c4daae0
add ECS geo fields into both client and server attribute
kaiyan-sheng ff6ba14
add links to reference
kaiyan-sheng File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we expect any client.geo and server.geo attributes to be present simultaneously? I thought the purpose of a global attribute registry is to represent attributes that mean the same wherever they are used, and that suits well for the geo.* attributes. That is, geo.city_name means the same whether it's used in the context of client or server or any other that's described at the end at https://www.elastic.co/guide/en/ecs/current/ecs-geo.html
Even if the client.geo.x and server.geo.x attributes are required in the same object, I think this problem is unsolved currently - for eg., one cannot have two
url.full
attributes in the same object if I have to represent the referring url or the url being redirected to in the case of 302.I think we should omit the
client
andserver
prefix.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.
Why not? From a web-server access log you can easily derive both and can be of interest for observability users.
That's right! But, the meaning doesn't change if you put an attribute into a more specific context, right?
geo.city_name
describes a city.client.geo.city_name
still describes a city, but the city of the client, whileserver.geo.city_name
describes the city of the server. And both can coexist on a single signal / event. I agree though that we should not definegeo.city_name
multiple times in different places, and that's what #339 aims at. I also created a concrete proposal on how to model it yaml: open-telemetry/build-tools#240@scheler This is exactly what we are trying to solve with #339. For the geo example that means:
geo.*
fields only once in the registrygeo.*
fields will be (explicitly) reusable under different namespaces (while keeping their original definition just applied into the context of the target namespace). This will result inclient.geo.*
andserver.geo.*
anyways.client
,server
prefixes for geo, we would break ECS with no reason (no actual conflict with semconv or attribute naming guidelines), which has been explicitly stated as something to avoid whenever possible in the original OTEP.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.
@AlexanderWert thanks for the clarification. One issue though is that one cannot use
geo.city_name
itself. Is that intended here?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.
@scheler Yes it's intended. In Elastic common schema,
geo
fields are not expected to be used directly at the root of the events.