feat(spans): Allow ingestion of metrics summary on spans #2823
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jjbayer
reviewed
Dec 7, 2023
Comment on lines
145
to
180
| // Check feature flag. | ||
| if !state | ||
| .project_state | ||
| .has_feature(Feature::SpanMetricsExtraction) | ||
| { | ||
| // When span metrics extraction is disabled, we need to check if DDM is enabled | ||
| // to still index spans with _metrics_summary | ||
| if !state.project_state.has_feature(Feature::CustomMetrics) { | ||
| return; | ||
| } | ||
|
|
||
| let Some(event) = state.event.value() else { | ||
| return; | ||
| }; | ||
|
|
||
| // Metrics summary is empty so we don't need to ingest the span. | ||
| if event._metrics_summary.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| // Extract transaction as a span. | ||
| let mut transaction_span: Span = event.into(); | ||
|
|
||
| // Extract tags to add to this span as well | ||
| let shared_tags = tag_extraction::extract_shared_tags(event); | ||
| transaction_span.sentry_tags = Annotated::new( | ||
| shared_tags | ||
| .clone() | ||
| .into_iter() | ||
| .map(|(k, v)| (k.sentry_tag_key().to_owned(), Annotated::new(v))) | ||
| .collect(), | ||
| ); | ||
| add_span(transaction_span.into()); | ||
|
|
||
| return; | ||
| }; |
Member
There was a problem hiding this comment.
Do we really need this extra logic? Span metrics extraction is at 100% anyway, right? Or are we trying to run this experiment on other instances (S4S)?
Contributor
Author
There was a problem hiding this comment.
We're trying to run this on S4S, where span metrics extraction is not enabled.
Comment on lines
165
to
177
| // Extract transaction as a span. | ||
| let mut transaction_span: Span = event.into(); | ||
|
|
||
| // Extract tags to add to this span as well | ||
| let shared_tags = tag_extraction::extract_shared_tags(event); | ||
| transaction_span.sentry_tags = Annotated::new( | ||
| shared_tags | ||
| .clone() | ||
| .into_iter() | ||
| .map(|(k, v)| (k.sentry_tag_key().to_owned(), Annotated::new(v))) | ||
| .collect(), | ||
| ); | ||
| add_span(transaction_span.into()); |
Member
There was a problem hiding this comment.
Even if we need this extra logic (see comment below), I think we could prevent copying this snippet and instead have something like
let extract_transaction_span = extraction_enabled || (custom_metrics_enabled && !summary.is_empty());
let extract_child_spans = extraction_enabled;
Contributor
Author
There was a problem hiding this comment.
True. I refactored it.
jjbayer
reviewed
Dec 11, 2023
Member
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
Let's also add a test to verify that spans are extracted when custom metrics are enabled (can be either a unit test of extract_from_event or a copy-paste of test_span_extraction).
jjbayer
approved these changes
Dec 12, 2023
jan-auer
approved these changes
Dec 12, 2023
jan-auer
added a commit
that referenced
this pull request
Dec 19, 2023
* master: (35 commits) fix(spans): Parse quotes in MySQL (#2846) ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter (#2849) fix(spans): Detect hex with fallback scrubber (#2868) release: 23.12.0 Revert "ci: Update upload-artifact and download-artifact actions" (#2866) Revert "build: Update axum and http" (#2863) feat(spans): Allow resource.img spans (#2855) build: Update axum and http (#2844) fix(build): Add additional dependencies to the release build (#2858) ci: Update upload-artifact and download-artifact actions (#2861) feat(spans): Parse timestamps from strings (#2857) fix(spans): Scrub integer file extensions (#2856) feat(spans): Remove unused transaction tag from resource metrics (#2853) ref(cardinality): Recover buckets on cardinality limiter failure (#2852) feat(server): Org rate limit per metric bucket (#2836) ref(spans): List metric tags explicitly (#2834) feat(spans): Resource response sizes as measurements (#2845) feat(crons): Add thresholds to monitor config payload (#2842) feat(spans): Allow ingestion of metrics summary on spans (#2823) ref(crons): Add documentation to CheckInMessageType (#2840) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to ingest metrics summary on spans, we need to add the field to the protocol and copy it during extraction.
On top of that, since span extraction is not always enabled when DDM is, we check if we need to persist the span by looking if it contains a non-empty metrics summary value and then persist the span if it does.