-
Notifications
You must be signed in to change notification settings - Fork 3k
[translator/azurelogs] Azure logs fix resource attributes #39571
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
[translator/azurelogs] Azure logs fix resource attributes #39571
Conversation
…; remove redundant casting
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.
This looks like the right choice but does have user facing changes that we should highlight (eg some log record attributes are moved to resource attributes). Also, I don't think we should remove event.name
attribute yet.
Consumers of these attributes may depend on their names and locations. Maybe we dual write them for a short time, or add a feature flag to control behaviour?
Thanks @MikeGoldsmith @cparkins @atoulme . I am not totally convinced on this @MikeGoldsmith Can you tell me more about the semantic conventions you linked for azure? Is this something that is in use in general, or just part of this translator? Reason I ask is that some of these attributes should not be there (for example Can we get this PR approved and merged so I can remove the log record body to now be part of the log record attributes as part of #39186? |
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 agree the updated structure is a step in the right direction. As the azure logs translator and associated semconv are marked as development, I think we can be a little more proactive with these changes.
Regarding the linked semconv, it's something @lmolkova and I worked on last year. It's only used in this translator as far as I know. It's there so we have a defined structure we expect the data to be in when converting into OTel events and could be used by other components in the future.
I think this is good enough for now, and we can revisit the event.name
attribute later.
@atoulme can you merge this please? To unblock progress on the log structure |
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
…etry#39571) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The issue for this PR was first reported here: open-telemetry#39186. Currently, the resource logs have no attributes. This means that all records are supposedly the same, which is not true. This PR adds attributes to the resources. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Relates open-telemetry#39186. <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests fixed.
…etry#39571) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The issue for this PR was first reported here: open-telemetry#39186. Currently, the resource logs have no attributes. This means that all records are supposedly the same, which is not true. This PR adds attributes to the resources. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Relates open-telemetry#39186. <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests fixed.
…etry#39571) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The issue for this PR was first reported here: open-telemetry#39186. Currently, the resource logs have no attributes. This means that all records are supposedly the same, which is not true. This PR adds attributes to the resources. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Relates open-telemetry#39186. <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests fixed.
Description
The issue for this PR was first reported here: #39186.
Currently, the resource logs have no attributes. This means that all records are supposedly the same, which is not true.
This PR adds attributes to the resources.
Link to tracking issue
Relates #39186.
Testing
Unit tests fixed.