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: Add OpenTelemetry integration #149

Closed

Conversation

sethmaxwl
Copy link

@sethmaxwl sethmaxwl commented Jul 9, 2020

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)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2020
@sethmaxwl sethmaxwl changed the title Add OpenTelemetry integration feat: Add OpenTelemetry integration Jul 9, 2020
@plamut
Copy link
Contributor

plamut commented Jul 10, 2020

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.

@plamut plamut added the status: blocked Resolving the issue is dependent on other work. label Jul 10, 2020
@sethmaxwl
Copy link
Author

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

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

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/publisher/client.py Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
tests/unit/pubsub_v1/opentelemetry/test_opentelemetry.py Outdated Show resolved Hide resolved
tests/unit/pubsub_v1/publisher/test_publisher_client.py Outdated Show resolved Hide resolved
@sethmaxwl
Copy link
Author

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.

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

No space between "message" and "as".

@plamut plamut requested a review from cguardia August 28, 2020 10:00
@cguardia cguardia added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2020
Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

"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."
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

# ----------------------------------------------------------------------------
# Additional unit test dependincies
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: dependencies.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

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

@plamut
Copy link
Contributor

plamut commented Aug 31, 2020

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
Copy link

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
Copy link
Contributor

plamut commented Aug 31, 2020

@johnbryan That sounds good, thanks!

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

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

plamut commented Sep 14, 2020

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

@anguillanneuf
Copy link
Contributor

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

@pamarc
Copy link

pamarc commented Nov 30, 2020

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

@plamut
Copy link
Contributor

plamut commented Mar 9, 2021

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

@plamut
Copy link
Contributor

plamut commented Aug 4, 2021

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.

@plamut plamut added the status: blocked Resolving the issue is dependent on other work. label Aug 4, 2021
@plamut 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 plamut changed the base branch from master to main August 25, 2021 10:18
@plamut plamut requested review from a team as code owners August 25, 2021 10:18
@plamut plamut changed the base branch from main to master August 26, 2021 11:53
@pradn pradn added the release-please:force-run To run release-please label Mar 9, 2022
@release-please release-please bot removed the release-please:force-run To run release-please label Mar 9, 2022
@CyberHippo
Copy link

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

@dmallanb
Copy link

dmallanb commented Sep 15, 2023

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)

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Closing this, since its now done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: blocked Resolving the issue is dependent on other work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PubSub add OpenTelemetry support