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

(Bluesky #1682) add OpenTelemetry tracing to status #1189

Merged
merged 12 commits into from
Jul 16, 2024

Conversation

d-perl
Copy link
Contributor

@d-perl d-perl commented Apr 17, 2024

To go along with bluesky/bluesky#1724

  • Adds opentelemetry-api as a dependency
  • Registers a trace on __init__ of StatusBase objects, ends it when they are marked done, and attaches some metadata to help identify what the status belongs to - this metadata is added for class specific metadata in each subclass
  • Registers traces on StatusBase.wait()

@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch 6 times, most recently from a44955c to b59688d Compare April 19, 2024 10:41
  - Adds opentelemetry-api as a dependency
  - Registers traces on __init__ of Status objects,
    ending when they are marked done
  - Registers traces on Status.wait()
@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch from b59688d to 77cbea4 Compare April 19, 2024 12:12
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

Looks like the SubscriptionStatus and DeviceStatus (and their subclasses) are not included. Was it intentional?

Why not to apply the changes to the StatusBase class?

docs/conf.py Outdated
@@ -104,7 +104,7 @@
# Example configuration for intersphinx: refer to the Python standard library.
intersphinx_mapping = {
"python": ("https://docs.python.org/3", None),
"bluesky": ("https://blueskyproject.io/bluesky/", None),
"bluesky": ("https://blueskyproject.io/bluesky/main", None),
Copy link
Member

@mrakitin mrakitin May 2, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Probably worth a separate PR.

@d-perl
Copy link
Contributor Author

d-perl commented Jun 10, 2024

Hi @mrakitin , thanks for your review, sorry it took me so long to get back to you! The trace is indeed applied to StatusBase, I have updated the PR description to be more clear about this. For e.g. DeviceStatus the device is added as an attribute.

@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch from 4236ea1 to 18e9a5d Compare June 10, 2024 11:59
@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch from bdabfe2 to b9d74ca Compare June 10, 2024 12:49
@mrakitin
Copy link
Member

@dperl-dls, are the CI failures expected?

@d-perl
Copy link
Contributor Author

d-perl commented Jun 27, 2024

@mrakitin yes, they're failing on master, unrelated to these changes. I can run all the tests locally and they pass, but I don't really know what I could do about the NSLS epics IOC containers. I was hoping this would be fixed by now and I could rebase.

@mrakitin
Copy link
Member

@dperl-dls, are the CI failures expected?

I think we need to refresh the image https://hub.docker.com/r/nsls2/epics-docker/tags to unblock the tests.

@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch 4 times, most recently from afc5269 to e1d1f36 Compare July 3, 2024 15:09
@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch 9 times, most recently from 977dbdf to 3ad76a1 Compare July 3, 2024 15:30
@d-perl d-perl force-pushed the bluesky_1682_add_opentelemetry_tracing branch from 3ad76a1 to b38c062 Compare July 3, 2024 15:33
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

Looks straightforward.

Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mrakitin mrakitin merged commit e9f51dd into bluesky:master Jul 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants