-
Notifications
You must be signed in to change notification settings - Fork 540
Recommeded resource attributes #3797
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
Recommeded resource attributes #3797
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.
LGTM. Please fix the CI
5103776
to
b41b9da
Compare
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.
We discussed about this PR during the SIG call. Some comments:
- Could you add a feature gate to avoid the breaking change right now and give some time to users to upgrade?
- Are the semantic convention changes already released?
- Is there something we can do to avoid the breaking change?
component: auto-instrumentation | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Update recommended resource attributes to match the [semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/k8s-attributes.md) |
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.
Please add more info about the exact changes that need to be done for the operator.
Also, provide some kind of recommendation regarding upgrading.
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.
the subtext has all changes - is that sufficient?
The subtext should also cover what you need to consider for upgrading.
yes, we could - but I'm not sure if that's really warranted in this case:
See Having said that, I'm happy to add a flag if we decide to do so
we could add a different flag for the well-known attributes and deprecate the old flag |
@iblancasa please check again |
just found a missing piece: |
503c19a
to
8d35a59
Compare
@iblancasa now it should be complete 😄 |
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 am ok with this going in without a feature flag, but I'd like opinions from other maintainers before we merge.
I thing we should include this using a feature gate to give users some time to migrate. But not strong opinion. |
Rather than a feature gate, I think I'd prefer some instructions on how to get the old behaviour by changing resource labels. |
I've added a section in the changelog |
@iblancasa @swiatekm please check again |
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.
looks great, thanks for your work here @zeitlinger !
Description:
Updates the recommended resource attributes to comply with the semantic conventions.
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/k8s-attributes.md
Technically, it's a breaking change - looking for feedback if it's OK to change as is.
Link to tracking Issue(s):
Testing:
Adapted existing tests.
Documentation:
Adapted existing docs.