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.

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

Changes Requested for ActiveSupport instrumentation #957

Open
7 tasks
arielvalentin opened this issue Apr 30, 2024 · 1 comment
Open
7 tasks

Changes Requested for ActiveSupport instrumentation #957

arielvalentin opened this issue Apr 30, 2024 · 1 comment
Labels
help wanted Extra attention is needed instrumentation keep Ensures stale-bot keeps this issue/PR open

Comments

@arielvalentin
Copy link
Collaborator

arielvalentin commented Apr 30, 2024 •
edited by robbkidd
Loading

During the SIG meeting on 2024-04-30, I brought up a few challenges of working with the existing ActiveSupport instrumentation, that were brought to our attention while instrumenting ActionMailer: #887

There were issues with missing options, implementation and naming that makes it difficult to use and potentially error prone. The list below includes some of the changes I would like to see:

  • Support Setting Span Kind feat: ActiveSupport user specified span kind #1016
  • Default to using the AS event name instead of splitting and inverting event names
  • Allow users to provide a proc that formats the span name feat!: Custom ActiveSupport Span Names #1014
  • Ingress/egress spans should default to the semantic conventions; while internal spans default to AS event name
  • Switch to allow lists instead of deny (disallow...) lists and default to excluding PII
  • Prevent users from mutating the ActiveSupport payload when extracting span attributes in notification_payload_transform
  • Map Rails key names to semantic convention key names
@github-actions GitHub Actions
Copy link
Contributor

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.

@github-actions github-actions bot added the stale Marks an issue/PR stale label May 31, 2024
@arielvalentin arielvalentin added keep Ensures stale-bot keeps this issue/PR open help wanted Extra attention is needed and removed stale Marks an issue/PR stale labels May 31, 2024
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 16, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names
9bd4d99 
The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See open-telemetry#957
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 16, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names
bcefae2 
The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See open-telemetry#957
@arielvalentin arielvalentin mentioned this issue Jun 16, 2024
Merged
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 16, 2024
@arielvalentin
feat: ActiveSupport user specified span kind
c7a5829 
This will allow users to subscribe to notifications for Server Ingress, Messaging, or clients

See open-telemetry#957
@arielvalentin arielvalentin mentioned this issue Jun 16, 2024
Merged
arielvalentin added a commit that referenced this issue Jun 18, 2024
@arielvalentin @xuan-cao-swi
feat: ActiveSupport user specified span kind (#1016)
a9c45e7 
This will allow users to subscribe to notifications for Server Ingress, Messaging, or clients

See #957

Co-authored-by: Xuan <112967240+xuan-cao-swi@users.noreply.github.com>
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 20, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names
6aa3ec0 
The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See open-telemetry#957
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 20, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names
a649c5a 
The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See open-telemetry#957
arielvalentin added a commit to arielvalentin/opentelemetry-ruby-contrib that referenced this issue Jun 20, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names
2be7ea8 
The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See open-telemetry#957
arielvalentin added a commit that referenced this issue Jun 24, 2024
@arielvalentin
feat!: Custom ActiveSupport Span Names (#1014)
e14d6b0 
* feat!: Custom ActiveSupport Span Names

The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See #957

* squash: Bolt on a few things

* squash: would be great if the tests passed

* squash: Linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Extra attention is needed instrumentation keep Ensures stale-bot keeps this issue/PR open
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@arielvalentin