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

Improve pulsar instrumentation #8007

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 8, 2023

  • use standard messaging span name
  • replace message.type with experimental attribute messaging.pulsar.message.type, message.type is from rpc semantic conventions
  • replace net.sock.peer.addr that was filled with broker url with net.peer.name and net.peer.port

@laurit laurit requested a review from a team as a code owner March 8, 2023 14:12
@github-actions github-actions bot requested a review from theletterf March 8, 2023 14:13
@trask
Copy link
Member

trask commented Mar 8, 2023

cc @tjiuming

@tjiuming
Copy link
Contributor

tjiuming commented Mar 8, 2023

it creates a new PulsarRequest object when sending a message, which seems will reduce performance.
Everything else LGTM

.addAttributesExtractor(
MessagingAttributesExtractor.create(getter, MessageOperation.RECEIVE))
NetClientAttributesExtractor.create(new PulsarNetClientAttributesGetter()))
.buildConsumerInstrumenter(MessageTextMapGetter.INSTANCE);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably use the common messagingReceiveInstrumentationEnabled flag

Suggested change
.buildConsumerInstrumenter(MessageTextMapGetter.INSTANCE);
.setEnabled(ExperimentalConfig.get().messagingReceiveInstrumentationEnabled())
.buildConsumerInstrumenter(MessageTextMapGetter.INSTANCE);

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 you don't mind I would work on this in a subsequent PR. As far as I understand using the messagingReceiveInstrumentationEnabled isn't enough to make it comply with spec, it should also use span link.

@mateuszrzeszutek mateuszrzeszutek merged commit dedc4d3 into open-telemetry:main Mar 13, 2023
@laurit laurit deleted the pulsar branch July 6, 2023 17:32
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.

None yet

4 participants