Skip to content

CP-48995: Instrument XenAPI.py to submit a traceparent #5639

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 5, 2024
Merged

CP-48995: Instrument XenAPI.py to submit a traceparent #5639

merged 1 commit into from
Jun 5, 2024

Conversation

@GabrielBuica
Copy link
Contributor

Instrument XenAPI.py to submit the current traceparent back into xapi if it can import opentelemetry.

Currently, we don't see the traces of sm calling back to xapi using XenAPI.py. This will instrument XenAPI.py to pass a traceparent into xapi when opentelemetry is available.

edwintorok
edwintorok reviewed May 20, 2024
View reviewed changes
@GabrielBuica GabrielBuica force-pushed the private/dbuica/CP-48995 branch from 4c3dd6e to 90a4d9f Compare May 20, 2024 15:24
edwintorok
edwintorok reviewed May 22, 2024
View reviewed changes
@GabrielBuica GabrielBuica force-pushed the private/dbuica/CP-48995 branch 3 times, most recently from 9e820b3 to 902d4c0 Compare May 22, 2024 13:42
@pnfox
Copy link

pnfox commented May 22, 2024

Patrick here, I think the try except around the imports is fine, but I don't think the function definition for with_tracecontext should be defined there. My instinct is that this should be a method of the UDSTransport class. You can use the a statement like if 'opentelemetry' in sys.modules to check if the opentelemetry module has been loaded, or use the environment variable check again. We can define the with_tracecontext method as an empty method for the other classes

@GabrielBuica GabrielBuica force-pushed the private/dbuica/CP-48995 branch 2 times, most recently from 3be81ad to ce28236 Compare May 22, 2024 14:50
@GabrielBuica
Copy link
Contributor Author

BVT 198454 with and without tracing enabled.

@GabrielBuica GabrielBuica force-pushed the private/dbuica/CP-48995 branch from ce28236 to 3beae1f Compare May 29, 2024 10:36
@GabrielBuica
CP-48995: Instrument XenAPI.py to submit traceparent
e5bb639 
Instrument `XenAPI.py` to submit the current traceparent back into xapi
if it can import `opentelemetry`.

Currently, we don't see the traces of `sm` calling back to `xapi` using
`XenAPI.py`. This will instrument `XenAPI.py` to pass a traceparent into
`xapi` when opentelemetry is available.

Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
@GabrielBuica GabrielBuica force-pushed the private/dbuica/CP-48995 branch from 3beae1f to e5bb639 Compare June 4, 2024 14:47
@GabrielBuica GabrielBuica mentioned this pull request Jun 5, 2024
Closed
@GabrielBuica
Copy link
Contributor Author

199185 BVT+BST with all components enabled passed.

robhoes
robhoes reviewed Jun 5, 2024
View reviewed changes
python3/packages/observer.py
# are not overridden and will be the defined no-op functions.
span, patch_module = _init_tracing(observer_configs, observer_config_dir)

# If tracing is now operational, explicity set "OTEL_SDK_DISABLED" to "false".
Copy link
Member

Choose a reason for hiding this comment

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

"now" or "not"?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, double negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The double negation is a bit annoying, but I wanted to follow the opentelemetry standard...

@GabrielBuica
Copy link
Contributor Author

199182 BVT+BST with tracing disabled passed.

robhoes
robhoes approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

This is only implemented for connections to the unix socket, right?

@GabrielBuica
Copy link
Contributor Author

Yes, only for connections to the unix socket. Main reason is to see SM calling back into xapi.

I am addressing the others separately here #5672.

@robhoes robhoes merged commit 99e05fb into xapi-project:master Jun 5, 2024
@GabrielBuica GabrielBuica deleted the private/dbuica/CP-48995 branch January 8, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edwintorok edwintorok edwintorok left review comments

@robhoes robhoes robhoes approved these changes

@psafont psafont psafont approved these changes

+1 more reviewer

@pnfox pnfox pnfox approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@GabrielBuica @pnfox @robhoes @edwintorok @psafont