feat: Add OpenTelemetry integration by sethmaxwl · Pull Request #149 · googleapis/python-pubsub · GitHub

Closes #124
Requires. #158.

This PR adds OpenTelemetry tracing to publisher and subscriber clients.
This PR does not interfere with any underlying client functions. OpenTelemetry is an optional dependency that creates a trace that tracks a message from when it is published to when it is received by a subscriber.

Ongoing concerns

Adding OpenTelemetry to unit tests as external dependencies.

PR checklist

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

This requires dropping Python 2 support, right? (#131)

The samples PR is more or less ready to be merged (IMO) and we can then make one final Python 2 compatible release, and drop Python 2 support immediately afterwards.

This requires dropping Python 2 support, right? (#131)

Yes, OpenTelemetry does not support any Python version before 3.4.

Disclaimer - I would have to familiarize myself with the opentelemetry library more, thus I primarily focused on the code aspects, and less on the actual semantics. Added suggestions at places where the code could benefit from them.

Disclaimer - I would have to familiarize myself with the opentelemetry library more, thus I primarily focused on the code aspects, and less on the actual semantics. Added suggestions at places where the code could benefit from them.

All suggestions have been addressed.

LGTM now, although we might have to do some additional changes once #158 is merged.

If you have time, you might want to rebase your branch onto that PR branch and see if things work there, too.

You should be using a BatchSpanProcessor here. SimpleExportSpanProcessor exports all spans sequentially and will be very slow.

No space between "message" and "as".

There are still a couple of suggestions from previous reviews, though they are mostly documentation and log warning formatting issues.

There are no spaces at the end of these lines, which will cause words to be displayed together (e.g. couldnot, rather than could not).

Same issue from above, where spaces are missing at the end of the lines.

Typo: dependencies.

Just a suggestion: In addition to the import try, wouldn't it be possible to use an environment variable for explicitly disabling opentelemetry? That way the tests below would not need to mess with sys.modules.

Seems like lint is not passing, plus there are a few failing tests. Please review.

Side note - before merging this (after releasing a new major version, of course), we must also run a few benchmarks to verify that the tracing does not hinder the performance, especially throughput. Mentioning as a a reminder to ourselves.

Hi all, Seth (author of this PR) finished his Google internship in mid-August so I assume is not actively maintaining this PR now. I would be willing to address the remaining comments here but will not be able to get to it for a couple of weeks. Seth if I'm mistaken and you are indeed still following/working on this let us know, but I assume you have other things to do!

@johnbryan That sounds good, thanks!

No rush, though, as we need too release a new major version first anyway.

@johnbryan FYI, a new major version has been released, unblocking this PR (which needs an update now, however).

@johnbryan Will you have a chance to rebase this PR?

Hi, any news on this PR? This could be of use :)

@johnbryan What's the status of this PR, do we have an ETA for bringing it to completion? Thanks in advance!

Just as a quick status update, it is still not clear what exactly do want to be tracked, still awaiting the official specs. We can thus consider this PR blocked until further notice.

Hi, is there any update/ETA on this PR? :)

This PR has been open a while - any updates as to why it's still blocked?
(EDIT 12/12/23 - have since decided myself it's not a required change and my use case has a workaround)

What is the argument for doing this? It seems to me that, best case, it results in the duplication of data in the message. But in the worst case it actually leaks sensitive information into observability stack, just like logging the output of a SQL query would.

Closing this, since its now done.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2020

sethmaxwl changed the title Add OpenTelemetry integration feat: Add OpenTelemetry integration Jul 9, 2020

plamut added the status: blocked Resolving the issue is dependent on other work. label Jul 10, 2020

plamut reviewed Jul 10, 2020

View reviewed changes

feat: Add OpenTelemetry integration

6ebdf8e

plamut reviewed Jul 16, 2020

View reviewed changes

Comment thread google/cloud/pubsub_v1/publisher/client.py

sethmaxwl added 2 commits July 16, 2020 19:20

Add test for span context key warning

84b3a12

Merge branch 'master' into opentelemetry-integration

b629f9d

sethmaxwl mentioned this pull request Jul 17, 2020

Add OpenTelemetry Support googleapis/nodejs-pubsub#1061

Closed

busunkim96 closed this Jul 31, 2020

plamut reopened this Jul 31, 2020

Merge branch 'master' into opentelemetry-integration

a2d5037

aravinsiva mentioned this pull request Aug 11, 2020

feat: add opentelemetry tracing googleapis/python-bigquery#215

Merged

Rename subscriber span names

ce1bdda

AndrewAXue reviewed Aug 16, 2020

View reviewed changes

chrisrossi reviewed Aug 19, 2020

View reviewed changes

plamut requested a review from cguardia August 28, 2020 10:00

cguardia added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2020

cguardia approved these changes Aug 31, 2020

View reviewed changes

cguardia suggested changes Aug 31, 2020

View reviewed changes

plamut removed the status: blocked Resolving the issue is dependent on other work. label Sep 14, 2020

parthea assigned johnbryan Jul 18, 2021

plamut added the status: blocked Resolving the issue is dependent on other work. label Aug 4, 2021

plamut mentioned this pull request Aug 4, 2021

PubSub add OpenTelemetry support #124

Closed

plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 4, 2021

plamut changed the base branch from master to main August 25, 2021 10:18

plamut requested review from a team August 25, 2021 10:18

plamut changed the base branch from main to master August 26, 2021 11:53

quartzmo mentioned this pull request Sep 9, 2021

pubsub: Add OpenTelemetry tracing googleapis/google-cloud-ruby#13960

Closed

owais mentioned this pull request Sep 14, 2021

OpenTelemetry Falcon 4.x integration falconry/falcon#1828

Open

mikesmith37 mentioned this pull request Nov 27, 2021

Open Telemetry Tracing Support #543

Closed

pradn added the release-please:force-run To run release-please label Mar 9, 2022

release-please Bot removed the release-please:force-run To run release-please label Mar 9, 2022

pedroserrudo approved these changes Sep 18, 2023

View reviewed changes

dmaier-FN reviewed Dec 12, 2023

View reviewed changes

mukund-ananthu closed this Sep 29, 2024

+                              if "googclient_OpenTelemetrySpanContext" in attrs:
+                                  _LOGGER.warning(
+                                      "googclient_OpenTelemetrySpanContext set on message"
+                                      "as an attribute, but will be overridden."

+                              "A parent span was provided but it could not be"
+                              "converted into a SpanContext. Ensure that the"
+                              "parent is a mapping with at least a trace_id, span_id"
+                              "and is_remote keys."

+                  trace.set_tracer_provider(TracerProvider())
+                  trace.get_tracer_provider().add_span_processor(
+                      SimpleExportSpanProcessor(CloudTraceSpanExporter())

+                      "opentelemetry-api and opentelemetry-instrumentation"
+                      "pip modules. See also"
+                      "https://opentelemetry-python.readthedocs.io/en/stable/getting-started.html"
+                  )

               python.py_samples()
+              # ----------------------------------------------------------------------------
+              # Additional unit test dependincies

-                          data=data, ordering_key=ordering_key, attributes=attrs
-                      )
+                      span_name = "{} publisher".format(topic)
+                      span_attributes = {"data": data.decode()}

feat: Add OpenTelemetry integration by sethmaxwl · Pull Request #149 · googleapis/python-pubsub · GitHub

Uh oh!

feat: Add OpenTelemetry integration - #149

feat: Add OpenTelemetry integration#149sethmaxwl wants to merge 5 commits intogoogleapis:maingoogleapis/python-pubsub:mainfrom sethmaxwl:opentelemetry-integrationCopy head branch name to clipboard

Conversation

sethmaxwl commented Jul 9, 2020 • edited by plamut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ongoing concerns

PR checklist

Uh oh!

plamut commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sethmaxwl commented Jul 10, 2020

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethmaxwl commented Jul 13, 2020

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewAXue Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

chrisrossi Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia left a comment

Choose a reason for hiding this comment

Uh oh!

cguardia Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia left a comment

Choose a reason for hiding this comment

Uh oh!

plamut commented Aug 31, 2020

Uh oh!

johnbryan commented Aug 31, 2020

Uh oh!

plamut commented Aug 31, 2020

Uh oh!

plamut commented Sep 14, 2020

Uh oh!

anguillanneuf commented Oct 12, 2020

Uh oh!

pamarc commented Nov 30, 2020

Uh oh!

plamut commented Mar 9, 2021

Uh oh!

plamut commented Aug 4, 2021

Uh oh!

CyberHippo commented Aug 8, 2022

Uh oh!

dmallanb commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaier-FN Dec 12, 2023

feat: Add OpenTelemetry integration#149
sethmaxwl wants to merge 5 commits into
googleapis:mainfrom
sethmaxwl:opentelemetry-integration

sethmaxwl commented Jul 9, 2020 •
edited by plamut
Loading

plamut commented Jul 10, 2020 •
edited
Loading

dmallanb commented Sep 15, 2023 •
edited
Loading