-
Notifications
You must be signed in to change notification settings - Fork 860
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
Update semconv to 1.21.0, PART 1 - KAFKA #9371
Conversation
// TODO (trask) does this have a replacement? | ||
// satisfies( | ||
// SemanticAttributes.MESSAGING_KAFKA_SOURCE_PARTITION, | ||
// AbstractLongAssert::isNotNegative), |
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.
@pyohannes @lmolkova @joaopgrassi I couldn't tell if it was intentional that there is no replacement for messaging.kafka.source.partition
?
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.
was removed in open-telemetry/semantic-conventions#100
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.
what I wasn't sure about is that open-telemetry/semantic-conventions#156 introduces replacements for the other attributes removed in open-telemetry/semantic-conventions#100
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.
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.
For the time being I think we should not remove that attribute we can just keep using stringKey("messaging.kafka.source.partition")
until the situation clarifies.
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.
If I remember correctly, when we agreed to remove the source
namespace, we bumped into that one and we couldn't come up with use cases where it was used in that way, but I agree we probably should clarify. I will add this to the agenda so it can be discussed in the next SIG meeting (tomorrow).
attributes.put(SemanticAttributes.MESSAGING_KAFKA_SOURCE_PARTITION, partition); | ||
// TODO (trask) does this have a replacement? | ||
// } else { | ||
// attributes.put(SemanticAttributes.MESSAGING_KAFKA_SOURCE_PARTITION, partition); | ||
} |
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 whole point of removing source
namespace was to write (well, there are a couple of other related reasons)
attributes.put(SemanticAttributes.MESSAGING_KAFKA_DESTINATION_PARTITION, partition);
instead of
if (camelDirection == CamelDirection.OUTBOUND) {
attributes.put(SemanticAttributes.MESSAGING_KAFKA_DESTINATION_PARTITION, partition);
} else {
attributes.put(SemanticAttributes.MESSAGING_KAFKA_SOURCE_PARTITION, partition);
}
enjoy ;)
Superseded by #9408 |
Splitting #9370 this up into smaller pieces for review. Once the parts are approved, I will merge them into a single PR, with one commit per part, and send any additional final commits if needed to make CI happy.