Skip to content

Conversation

@tomerqodo
Copy link

Benchmark PR getsentry#103913

Type: Clean (correct implementation)

Original PR Title: feat(segment-enrichment): Record seen segment names
Original PR Description: Continues the work started in getsentry#103739.

As a first step to adding incremental segment name clustering to segment enrichment, record each seen segment name (like we do for transaction names in Sentry's event_manager). To work around the different signatures for transactions and segment spans, I extracted as much of the logic as I could into a second function, with one caller per scenario.

As with the previous PR, the changes are behind an org flag (organizations:normalize_segment_names_in_span_enrichment) for testing and easy rollback.

Closes ENG-5951.
Original PR URL: getsentry#103913

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds functionality to record seen segment names for incremental clustering, following the same pattern used for transaction names. The changes extract common logic into a shared function and integrate segment name recording into the span enrichment pipeline, all behind a feature flag for safe rollout.

Key Changes:

  • Adds record_segment_name() function to record segment names into Redis for clustering
  • Extracts common validation logic into _should_store_segment_name_inner() shared by both transactions and segments
  • Integrates segment name recording into the span processing pipeline with feature flag protection

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/sentry/ingest/transaction_clusterer/datasource/redis.py Adds segment name recording function and refactors validation logic into shared function
src/sentry/spans/consumers/process_segments/message.py Integrates segment name recording call after normalization with feature flag check
src/sentry/ingest/transaction_clusterer/normalization.py Uses constant for transaction source instead of string literal
tests/sentry/ingest/test_transaction_clusterer.py Adds comprehensive parametrized tests for segment name recording behavior
tests/sentry/spans/consumers/process_segments/test_message.py Adds mocks to verify segment name recording is called appropriately
Comments suppressed due to low confidence (1)

tests/sentry/spans/consumers/process_segments/test_message.py:318

  • The test test_segment_name_normalization_checks_source verifies that segment names with source "route" are not normalized, but it doesn't verify whether record_segment_name is called or not. Based on the test coverage in test_record_segment_name, source "route" should not be recorded (expected=0). Consider adding a mock assertion here to verify that record_segment_name is either not called or called but does not record anything for route sources.
    def test_segment_name_normalization_checks_source(self):
        _, segment_span = self.generate_basic_spans()
        segment_span["name"] = "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user/123/0"
        segment_span["attributes"][ATTRIBUTE_NAMES.SENTRY_SPAN_SOURCE] = {
            "type": "string",
            "value": "route",
        }

        with self.feature("organizations:normalize_segment_names_in_span_enrichment"):
            processed_spans = process_segment([segment_span])

        assert (
            processed_spans[0]["name"] == "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user/123/0"
        )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bar-qodo
Copy link

bar-qodo commented Jan 8, 2026

@copilot /review

Copy link

Copilot AI commented Jan 8, 2026

@bar-qodo I've opened a new pull request, #99, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants