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.

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

feat: otel updates for latest spec doc #5

Merged
merged 14 commits into from
Mar 22, 2024
Merged

feat: otel updates for latest spec doc #5

merged 14 commits into from
Mar 22, 2024

Conversation

feywind
Copy link
Owner

@feywind feywind commented Mar 13, 2024

Creating this for easier review on the final spec updates.

src/telemetry-tracing.ts Outdated Show resolved Hide resolved
src/telemetry-tracing.ts Outdated Show resolved Hide resolved
}
if (message.orderingKey) {
spanAttributes['messaging.gcp_pubsub.message.ordering_key'] =
spanAttributes['messaging.gcp_pubsub.ordering_key'] =

Choose a reason for hiding this comment

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

Actually the original was right: https://opentelemetry.io/docs/specs/semconv/messaging/gcp-pubsub/

Fixed in the doc

@@ -55,6 +55,23 @@ export interface PubsubMessage
* @private
*/
publishSchedulerSpan?: tracing.Span;

/**
* If this is a message being received from a subscription, expose the ackId.

Choose a reason for hiding this comment

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

I think expose here just means expose to the other parts of the client library and not users right?

Copy link
Owner Author

@feywind feywind Mar 22, 2024

Choose a reason for hiding this comment

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

Yep. I need to make a whole separate pass of converting @private to @internal since tsdoc changed that.

@@ -111,6 +111,7 @@ export abstract class MessageQueue extends EventEmitter {
}
spanMessages.forEach(m => {
tracing.PubsubSpans.updatePublisherTopicName(m.parentSpan!, topic.name);

Choose a reason for hiding this comment

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

Technically not part of this PR but why is this necessary? It looks like the same information is already passed when createPublisherSpan is called.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Mentioned over chat, but the trouble is that, since everything is async, it's possible that the code could get to the part that sets the initial span name before the project ID is resolved. In that case, the project ID will become {{project}} in the final span, which isn't great. The project ID must be resolved by this point, so this just makes sure the right names are in place.

@feywind
Copy link
Owner Author

feywind commented Mar 22, 2024

Merging this into the upstream PR branch.

@feywind feywind merged commit 000fd12 into otel-4 Mar 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants