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

Add ActiveStorage Instrumentation #2081

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cwoodcox
Copy link
Contributor

@cwoodcox cwoodcox commented Jun 10, 2022

An updated version of #1290, this adds ActiveStorage instrumentation via ActiveSupport::Notifications just like the other Rails integrations.

To-do

  • add @public_api tags to the proper methods/modules to match the rest
  • add documentation comments for the individual event modules
  • change span_name in all events to be the event name (delete, exist, etc)
  • change resource name to be blob name
    • include storage service name?
  • add tests

moved and re-namespaced everything
@cwoodcox cwoodcox requested a review from a team June 10, 2022 19:51
@cwoodcox cwoodcox force-pushed the contrib-active-storage branch from 5a3c6f4 to 9c49ecf Compare June 10, 2022 21:45
Copy link
Member

@marcotc marcotc 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 an awesome PR, super flushed out already, thank you @cwoodcox! 🙇
With small tweaks and testing this can be merged very soon.

I left an inline comment for us to discuss.

Besides testing, one thing missing is user-facing documentation, which goes in the https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md file. You can certainly leave it until the end to add it though.

# checksum is also on payload but this has infinite cardinality and low value

span.service = configuration[:service_name] if configuration[:service_name]
span.resource = "#{as_service}: #{as_key}"
Copy link
Member

Choose a reason for hiding this comment

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

I believe as_key has high cardinality, as it seems to be unique per file/path stored.
This falls under the same unresolved issue we have with instrumenting URL paths for arbitrary URLs: there's no reliable default quantizer we can think of that wouldn't cause high cardinality for some users.

I recommend the removal of as_key here, and instead set it as a tag (span.set_tag(...,as_key)).

Please let me know if my assumptions are incorrect and as_key is either a predictable value or can be quantized reliably.

@marcotc marcotc added integrations Involves tracing integrations community Was opened by a community member labels Jun 20, 2022
@ivoanjo
Copy link
Member

ivoanjo commented Dec 9, 2024

This PR has been open for quite a while; @cwoodcox is this something you're still interested in pushing forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants