NotificationsYou must be signed in to change notification settings
Fork213
Star425

feat: Add OpenTelemetry integration#149

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

sethmaxwl wants to merge5 commits intogoogleapis:mainfromsethmaxwl:opentelemetry-integration

Closed

feat: Add OpenTelemetry integration#149

sethmaxwl wants to merge5 commits intogoogleapis:mainfromsethmaxwl:opentelemetry-integration

Conversation

Copy link

sethmaxwl commentedJul 9, 2020•
edited by plamut
Loading

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 added the cla: yesThis 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

Copy link

Contributor

plamut commentedJul 10, 2020•
edited
Loading

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

Thesamples 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 added the status: blockedResolving the issue is dependent on other work. label

Jul 10, 2020

Copy link

Author

sethmaxwl commentedJul 10, 2020

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

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

sethmaxwl force-pushed theopentelemetry-integration branch from74bbff1 tod30d4f2Compare

July 10, 2020 15:09

plamut reviewed

Jul 10, 2020

View reviewed changes

Copy link

Contributor

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 theopentelemetry 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 OutdatedShow resolvedHide resolved

google/cloud/pubsub_v1/publisher/client.pyShow resolvedHide resolved

google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py OutdatedShow resolvedHide resolved

noxfile.pyShow resolvedHide resolved

tests/unit/pubsub_v1/opentelemetry/test_opentelemetry.py OutdatedShow resolvedHide resolved

tests/unit/pubsub_v1/publisher/test_publisher_client.py OutdatedShow resolvedHide resolved

feat: Add OpenTelemetry integration

6ebdf8e

sethmaxwl force-pushed theopentelemetry-integration branch from54be418 to6ebdf8eCompare

July 13, 2020 21:32

Copy link

Author

sethmaxwl commentedJul 13, 2020

Disclaimer - I would have to familiarize myself with theopentelemetry 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 reviewed

Jul 16, 2020

View reviewed changes

Copy link

Contributor

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.

plamut reviewed

Jul 16, 2020

View reviewed changes

google/cloud/pubsub_v1/publisher/client.pyShow resolvedHide resolved

Add test for span context key warning

84b3a12

sethmaxwl force-pushed theopentelemetry-integration branch frome237daf to84b3a12Compare

July 16, 2020 19:20

Merge branch 'master' into opentelemetry-integration

b629f9d

sethmaxwl mentioned this pull request

Jul 17, 2020

Add OpenTelemetry Supportgoogleapis/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 tracinggoogleapis/python-bigquery#215

Merged

Rename subscriber span names

ce1bdda

AndrewAXue reviewed

Aug 16, 2020

View reviewed changes

README.rst


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

Copy link

AndrewAXueAug 16, 2020

Choose a reason for hiding this comment

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

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

chrisrossi reviewed

Aug 19, 2020

View reviewed changes

google/cloud/pubsub_v1/publisher/client.py

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

Copy link

chrisrossiAug 19, 2020

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 requested a review fromcguardia

August 28, 2020 10:00

cguardia added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Aug 31, 2020

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Aug 31, 2020

cguardia approved these changes

Aug 31, 2020

View reviewed changes

Copy link

Contributor

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.

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

Copy link

Contributor

cguardiaAug 31, 2020

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

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

Copy link

Contributor

cguardiaAug 31, 2020

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.

synth.py

		python.py_samples()

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

Copy link

Contributor

cguardiaAug 31, 2020

Choose a reason for hiding this comment

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

Typo: dependencies.

google/cloud/pubsub_v1/opentelemetry_tracing.py

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

Copy link

Contributor

cguardiaAug 31, 2020

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.

cguardia suggested changes

Aug 31, 2020

View reviewed changes

Copy link

Contributor

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.

Copy link

Contributor

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

Copy link

johnbryan commentedAug 31, 2020

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!

Copy link

Contributor

plamut commentedAug 31, 2020

@johnbryan That sounds good, thanks!

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

plamut removed the status: blockedResolving the issue is dependent on other work. label

Sep 14, 2020

Copy link

Contributor

plamut commentedSep 14, 2020

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

Copy link

Contributor

anguillanneuf commentedOct 12, 2020

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

Copy link

pamarc commentedNov 30, 2020

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

Copy link

Contributor

plamut commentedMar 9, 2021

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

parthea assignedjohnbryan

Jul 18, 2021

Copy link

Contributor

plamut commentedAug 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 added the status: blockedResolving 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 mergeIndicates a pull request not ready for merge, due to either quality or timing. label

Aug 4, 2021

plamut changed the base branch frommaster tomain

August 25, 2021 10:18

plamut requested review froma team ascode owners

August 25, 2021 10:18

plamut changed the base branch frommain tomaster

August 26, 2021 11:53

quartzmo mentioned this pull request

Sep 9, 2021

pubsub: Add OpenTelemetry tracinggoogleapis/google-cloud-ruby#13960

Closed

owais mentioned this pull request

Sep 14, 2021

OpenTelemetry Falcon 4.x integrationfalconry/falcon#1828

Open

mikesmith37 mentioned this pull request

Nov 27, 2021

Open Telemetry Tracing Support#543

Closed

pradn added the release-please:force-runTo run release-please label

Mar 9, 2022

release-pleasebot removed the release-please:force-runTo run release-please label

Mar 9, 2022

Copy link

CyberHippo commentedAug 8, 2022

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

Copy link

dmallanb commentedSep 15, 2023•
edited
Loading

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)

pedroserrudo approved these changes

Sep 18, 2023

View reviewed changes

dmaier-FN reviewed

Dec 12, 2023

View reviewed changes

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()}

Copy link

dmaier-FNDec 12, 2023

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 theworst case it actually leaks sensitive information into observability stack, just like logging the output of a SQL query would.