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

sethmaxwl · 2020-07-09T21:12:19Z

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)

plamut · 2020-07-10T14:19:44Z

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.

sethmaxwl · 2020-07-10T14:27:53Z

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

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

plamut

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.

sethmaxwl · 2020-07-13T21:34:53Z

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.

plamut

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.

AndrewAXue · 2020-08-16T03:42:19Z

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


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

chrisrossi · 2020-08-19T18:46:52Z

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


No space between "message" and "as".

cguardia

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

cguardia · 2020-08-31T08:07:22Z

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


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).

cguardia · 2020-08-31T08:09:46Z

+ "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."


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

cguardia · 2020-08-31T08:14:34Z

 python.py_samples()

+# ----------------------------------------------------------------------------
+# Additional unit test dependincies


Typo: dependencies.

cguardia · 2020-08-31T08:23:37Z

+ "pip modules. See also"
+ "https://opentelemetry-python.readthedocs.io/en/stable/getting-started.html"
+ )
+ USE_OPENTELEMETRY = False


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.

cguardia

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

plamut · 2020-08-31T09:49:28Z

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.

johnbryan · 2020-08-31T13:53:55Z

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!

plamut · 2020-08-31T14:01:23Z

@johnbryan That sounds good, thanks!

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

plamut · 2020-09-14T17:14:54Z

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

anguillanneuf · 2020-10-12T23:48:07Z

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

pamarc · 2020-11-30T17:32:48Z

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

plamut · 2021-03-09T18:43:56Z

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

plamut · 2021-08-04T15:09:18Z

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.

CyberHippo · 2022-08-08T12:43:12Z

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

dmallanb · 2023-09-15T13:09:46Z

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)

dmaier-FN · 2023-12-12T18:03:45Z

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


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.

mukund-ananthu · 2024-09-29T05:26:17Z

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 integrationJul 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 threadgoogle/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 mainAugust 25, 2021 10:18

plamut requested review from a teamAugust 25, 2021 10:18

plamut changed the base branch from main to masterAugust 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-pleaseBot 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

Uh oh!

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!

AndrewAXueAug 16, 2020

Choose a reason for hiding this comment

Uh oh!

chrisrossiAug 19, 2020

Choose a reason for hiding this comment

Uh oh!

cguardia left a comment

Choose a reason for hiding this comment

Uh oh!

cguardiaAug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardiaAug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardiaAug 31, 2020

Choose a reason for hiding this comment

Uh oh!

cguardiaAug 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-FNDec 12, 2023

Choose a reason for hiding this comment

Uh oh!

mukund-ananthu commented Sep 29, 2024

sethmaxwl commented Jul 9, 2020•
edited by plamut
Loading

plamut commented Jul 10, 2020•
edited
Loading

dmallanb commented Sep 15, 2023•
edited
Loading