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.

Sign up for GitHub

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

Jump to bottom

[chore] update wording of ecs recommendations #1272

Closed
wants to merge 2 commits into from
Closed

[chore] update wording of ecs recommendations #1272

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df9bf95
update wording of ecs recommendations
trisch-me Jul 23, 2024
7bb23f7
Merge branch 'main' into update_ecs
trisch-me Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,9 +366,11 @@ exists in some form in ECS, consider the following guidelines:
- When no suitable alternatives are provided, altering an ECS name solely
for the purpose of complying with [Name Pluralization guidelines](docs/general/attribute-naming.md#name-pluralization-guidelines)
MAY BE avoided.
- Do not use an existing ECS name as a namespace. If the name must differ, use a
different namespace name to avoid clashes or avoid using the namespace
entirely. See the [ECS field reference] for existing namespaces.
- We should aim to avoid using an existing ECS name as a namespace whenever
possible and feasible. However, in critical situations where no other good
option exists, such changes might be necessary. If the name must differ,
Comment on lines +369 to +371
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the slack discussion, it seems this limitation can be removed altogether. What would be the reason to keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make ecs compatibility with otel easier. This whole chapter was added for this goal - to consider existing ecs namespaces and attributes while advancing on otel during donation of ECS, we should aim to avoid this conflict as we do with other breaking changes.

Copy link
Contributor

@lmolkova lmolkova Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lost

Felix Barnsteiner 1 day ago
At Elastic, we’re soon moving to a model where we store OTel attributes as-is, and with a flat structure that avoids causing ingestion issues in these situations. There will be optional runtime/alias mappings back to ECS. So I don’t really see the issue here and why a change like this would be a breaking one.

Felix Barnsteiner 1 day ago
Changing file.owner to file.owner.id is a breaking change but it’s not worse than changing it to file.owner_id, for instance.
I may be missing some context here, and maybe there are some nuances like this making an ECS->OTel compatibility layer more challenging. Although these seem like technical challenges that we could overcome.

I'm reading this and the impression I get is that conflict with namespace is not a problem. Breaking change is, but conflict with namespace does not introduce new problems on top of breaking.

So if I'm reading this correctly, there is no reason to keep the limitation and previous points in this doc already advise against breaking changes to ECS. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we take the whole paragraph it is about preventing breaking changes in ecs. The first point is about avoiding breaking changes in attribute naming and second is avoiding breaking changes in attribute/namespace collisions. Those are different sorts of breaking changes and we should try to avoid them both if possible.
We also have the same rule (stricter one) for semconv itself:

Names SHOULD NOT coincide with namespaces. For example if service.instance.id is an attribute name then it is no longer valid to have an attribute named service.instance because service.instance is already a namespace. Because of this rule be careful when choosing names: every existing name prohibits existence of an equally named namespace in the future, and vice versa: any existing namespace prohibits existence of an equally named attribute key in the future.

I want to emphasise both cases therefore I have left both options and changed the wording

Copy link
Contributor

@lmolkova lmolkova Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second is avoiding breaking changes in attribute/namespace collisions.

but it seems there are no problem with attribute and namespace collisions in ECS, so why do we need this sentence?

Copy link
Member

@ChrsMark ChrsMark Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that it won't be a problem in the future but it's mostly a safety net to ensure a smoother transition of ECS users to the new schema. Note the following:

we’re soon moving to a model where we store OTel attributes as-is.

Specially the soon.

As I see it it's mostly a "soft" guidance to ensure that whenever a breaking-change/conflict of this type can be avoided there is this guidance to suggest it.

If we have a case where there is no strong preference between 2 names, then this ECS guidance could be applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it seems there are no problem with attribute and namespace collisions in ECS

this is still a problem and a breaking change as with attribute name changes. It's just not as severe as it was before

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why it is a problem. Based on the discussion in the chat it's not or it won't be soon.

use a different namespace name to avoid clashes or avoid using the
namespace entirely. See the [ECS field reference] for existing namespaces.

[nvm]: https://github.com/nvm-sh/nvm/blob/master/README.md#installing-and-updating
[stability guarantees]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.26.0/specification/versioning-and-stability.md#semantic-conventions-stability
Expand Down
Loading