Skip to content

fix(spans): Restore top-level _performance_issues_spans#6045

Merged
jjbayer merged 8 commits into
masterfrom
fix/perf-issues-spans
Jun 4, 2026
Merged

fix(spans): Restore top-level _performance_issues_spans#6045
jjbayer merged 8 commits into
masterfrom
fix/perf-issues-spans

Conversation

@jjbayer
Copy link
Copy Markdown
Member

@jjbayer jjbayer commented Jun 3, 2026

Restore top-level _performance_issues_spans, but make it part of the kafka message instead of the span schema.

Fixes INGEST-879

@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 3, 2026

INGEST-879

pub span_op_defaults: BorrowedSpanOpDefaults<'a>,

/// Set a flag to enable performance issue detection on spans.
pub performance_issues_spans: bool,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Setting this flag is now centralized in forward_store.

skip_serialization = "empty",
trim = false
)]
pub performance_issues_spans: Annotated<bool>,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This shouldn't be part of the span schema.

downsampled_retention_days: retentions.downsampled,
event_id: None,
item: span,
performance_issues_spans: false,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This flag is only used for segment spans created from transactions.

downsampled_retention_days: ctx.retention.downsampled,
event_id: None,
item: span,
performance_issues_spans: false,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This flag is only used for segment spans created from transactions.

retention_days: retentions.standard,
downsampled_retention_days: retentions.downsampled,
event_id,
performance_issues_spans: false,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the initial value, but we conditionally overwrite it in forward_store.

@jjbayer jjbayer marked this pull request as ready for review June 4, 2026 17:14
@jjbayer jjbayer requested a review from a team as a code owner June 4, 2026 17:14
Comment on lines +60 to +61
use relay_dynamic_config::Feature;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this on purpose?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nope, IDE did this because the function is guarded by feature = "processing".

Comment on lines +60 to +61
use relay_dynamic_config::Feature;

Copy link
Copy Markdown
Member

@tobias-wilfert tobias-wilfert Jun 4, 2026

Choose a reason for hiding this comment

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

Is there a reason to put this inside here but use relay_protocol::Annotated; outside? I don't mind either but think having both the same way makes sense?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nope, IDE did this because the function is guarded by feature = "processing".

if let Some(event) = work.event.value_mut()
&& performance_issues_spans
{
event.performance_issues_spans = Annotated::new(true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment on this is still /// Temporary flag that controls where performance issues are detected. Is the idea to eventually get rid of it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes it should eventually get removed.

@jjbayer jjbayer enabled auto-merge June 4, 2026 19:12
@jjbayer jjbayer added this pull request to the merge queue Jun 4, 2026
Merged via the queue into master with commit 90e78d4 Jun 4, 2026
33 checks passed
@jjbayer jjbayer deleted the fix/perf-issues-spans branch June 4, 2026 19:44
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