feat: Add OpenTelemetry integration #149

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.

google/cloud/pubsub_v1/opentelemetry_tracing.py

google/cloud/pubsub_v1/publisher/client.py

google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py

noxfile.py

tests/unit/pubsub_v1/opentelemetry/test_opentelemetry.py

tests/unit/pubsub_v1/publisher/test_publisher_client.py

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.

google/cloud/pubsub_v1/publisher/client.py

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

README.rst

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

google/cloud/pubsub_v1/publisher/client.py

+                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

google/cloud/pubsub_v1/opentelemetry_tracing.py

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

google/cloud/pubsub_v1/opentelemetry_tracing.py

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

synth.py

@@ -279,4 +279,17 @@ def _merge_dict(d1, d2):
 # ----------------------------------------------------------------------------
 python.py_samples()

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


Typo: dependencies.

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

google/cloud/pubsub_v1/opentelemetry_tracing.py

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

google/cloud/pubsub_v1/publisher/client.py

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

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

sethmaxwl force-pushed the opentelemetry-integration branch from 74bbff1 to d30d4f2 Compare July 10, 2020 15:09

plamut reviewed Jul 10, 2020

View reviewed changes

feat: Add OpenTelemetry integration

6ebdf8e

sethmaxwl force-pushed the opentelemetry-integration branch from 54be418 to 6ebdf8e Compare July 13, 2020 21:32

plamut reviewed Jul 16, 2020

View reviewed changes

google/cloud/pubsub_v1/publisher/client.py Outdated Show resolved Hide resolved

Add test for span context key warning

84b3a12

sethmaxwl force-pushed the opentelemetry-integration branch from e237daf to 84b3a12 Compare July 16, 2020 19:20

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 as code owners 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

Open

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add OpenTelemetry integration #149

feat: Add OpenTelemetry integration #149

sethmaxwl commented Jul 9, 2020 •

edited by plamut

plamut commented Jul 10, 2020 •

edited

sethmaxwl commented Jul 10, 2020

plamut left a comment

sethmaxwl commented Jul 13, 2020

plamut left a comment

AndrewAXue Aug 16, 2020

chrisrossi Aug 19, 2020

cguardia left a comment

cguardia Aug 31, 2020

cguardia Aug 31, 2020

cguardia Aug 31, 2020

cguardia Aug 31, 2020

cguardia left a comment

plamut commented Aug 31, 2020

johnbryan commented Aug 31, 2020

plamut commented Aug 31, 2020

plamut commented Sep 14, 2020

anguillanneuf commented Oct 12, 2020

pamarc commented Nov 30, 2020

plamut commented Mar 9, 2021

plamut commented Aug 4, 2021

CyberHippo commented Aug 8, 2022

dmallanb commented Sep 15, 2023 •

edited

dmaier-FN Dec 12, 2023

feat: Add OpenTelemetry integration #149

Are you sure you want to change the base?

feat: Add OpenTelemetry integration #149

Conversation

sethmaxwl commented Jul 9, 2020 • edited by plamut

Ongoing concerns

PR checklist

plamut commented Jul 10, 2020 • edited

sethmaxwl commented Jul 10, 2020

plamut left a comment

Choose a reason for hiding this comment

sethmaxwl commented Jul 13, 2020

plamut left a comment

Choose a reason for hiding this comment

AndrewAXue Aug 16, 2020

Choose a reason for hiding this comment

chrisrossi Aug 19, 2020

Choose a reason for hiding this comment

cguardia left a comment

Choose a reason for hiding this comment

cguardia Aug 31, 2020

Choose a reason for hiding this comment

cguardia Aug 31, 2020

Choose a reason for hiding this comment

cguardia Aug 31, 2020

Choose a reason for hiding this comment

cguardia Aug 31, 2020

Choose a reason for hiding this comment

cguardia left a comment

Choose a reason for hiding this comment

plamut commented Aug 31, 2020

johnbryan commented Aug 31, 2020

plamut commented Aug 31, 2020

plamut commented Sep 14, 2020

anguillanneuf commented Oct 12, 2020

pamarc commented Nov 30, 2020

plamut commented Mar 9, 2021

plamut commented Aug 4, 2021

CyberHippo commented Aug 8, 2022

dmallanb commented Sep 15, 2023 • edited

dmaier-FN Dec 12, 2023

Choose a reason for hiding this comment

sethmaxwl commented Jul 9, 2020 •

edited by plamut

plamut commented Jul 10, 2020 •

edited

dmallanb commented Sep 15, 2023 •

edited