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

Unrevert "feat(ourlogs): Allow log ingestion behind a flag"" #4471

Merged
merged 9 commits into from
Jan 29, 2025

Conversation

k-fish
Copy link
Member

@k-fish k-fish commented Jan 23, 2025

@k-fish k-fish requested a review from jjbayer January 23, 2025 21:39
@k-fish k-fish self-assigned this Jan 23, 2025
@k-fish k-fish requested a review from a team as a code owner January 23, 2025 21:39
Copy link

codecov bot commented Jan 23, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4 1 3 0
View the top 1 failed tests by shortest run time
_integration-test.test_01_basics::test_receive_event
Stack Traces | 69.5s run time
client_login = (<httpx.Client object at 0x7f0e97c2a240>, <Response [200 OK]>)

    #x1B[0m#x1B[94mdef#x1B[39;49;00m #x1B[92mtest_receive_event#x1B[39;49;00m(client_login):#x1B[90m#x1B[39;49;00m
        event_id = #x1B[94mNone#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
        client, _ = client_login#x1B[90m#x1B[39;49;00m
        #x1B[94mwith#x1B[39;49;00m sentry_sdk.init(dsn=get_sentry_dsn(client)):#x1B[90m#x1B[39;49;00m
            event_id = sentry_sdk.capture_exception(#x1B[96mException#x1B[39;49;00m(#x1B[33m"#x1B[39;49;00m#x1B[33ma failure#x1B[39;49;00m#x1B[33m"#x1B[39;49;00m))#x1B[90m#x1B[39;49;00m
        #x1B[94massert#x1B[39;49;00m event_id #x1B[95mis#x1B[39;49;00m #x1B[95mnot#x1B[39;49;00m #x1B[94mNone#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
>       response = poll_for_response(#x1B[90m#x1B[39;49;00m
            #x1B[33mf#x1B[39;49;00m#x1B[33m"#x1B[39;49;00m#x1B[33m{#x1B[39;49;00mSENTRY_TEST_HOST#x1B[33m}#x1B[39;49;00m#x1B[.../sentry/internal/events/#x1B[39;49;00m#x1B[33m{#x1B[39;49;00mevent_id#x1B[33m}#x1B[39;49;00m#x1B[33m/#x1B[39;49;00m#x1B[33m"#x1B[39;49;00m, client#x1B[90m#x1B[39;49;00m
        )#x1B[90m#x1B[39;49;00m

#x1B[1m#x1B[31m_integration-test/test_01_basics.py#x1B[0m:129: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

request = 'http://localhost:.../internal/events/bfc372747853403884edf92dbaa43bde/'
client = <httpx.Client object at 0x7f0e97c2a240>, validator = None

    #x1B[0m#x1B[94mdef#x1B[39;49;00m #x1B[92mpoll_for_response#x1B[39;49;00m(#x1B[90m#x1B[39;49;00m
        request: #x1B[96mstr#x1B[39;49;00m, client: httpx.Client, validator: Callable = #x1B[94mNone#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
    ) -> httpx.Response:#x1B[90m#x1B[39;49;00m
        #x1B[94mfor#x1B[39;49;00m i #x1B[95min#x1B[39;49;00m #x1B[96mrange#x1B[39;49;00m(TIMEOUT_SECONDS):#x1B[90m#x1B[39;49;00m
            response = client.get(#x1B[90m#x1B[39;49;00m
                request, follow_redirects=#x1B[94mTrue#x1B[39;49;00m, headers={#x1B[33m"#x1B[39;49;00m#x1B[33mReferer#x1B[39;49;00m#x1B[33m"#x1B[39;49;00m: SENTRY_TEST_HOST}#x1B[90m#x1B[39;49;00m
            )#x1B[90m#x1B[39;49;00m
            #x1B[94mif#x1B[39;49;00m response.status_code == #x1B[94m200#x1B[39;49;00m:#x1B[90m#x1B[39;49;00m
                #x1B[94mif#x1B[39;49;00m validator #x1B[95mis#x1B[39;49;00m #x1B[94mNone#x1B[39;49;00m #x1B[95mor#x1B[39;49;00m validator(response.text):#x1B[90m#x1B[39;49;00m
                    #x1B[94mbreak#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
            time.sleep(#x1B[94m1#x1B[39;49;00m)#x1B[90m#x1B[39;49;00m
        #x1B[94melse#x1B[39;49;00m:#x1B[90m#x1B[39;49;00m
>           #x1B[94mraise#x1B[39;49;00m #x1B[96mAssertionError#x1B[39;49;00m(#x1B[90m#x1B[39;49;00m
                #x1B[33m"#x1B[39;49;00m#x1B[33mtimeout waiting for response status code 200 or valid data#x1B[39;49;00m#x1B[33m"#x1B[39;49;00m#x1B[90m#x1B[39;49;00m
            )#x1B[90m#x1B[39;49;00m
#x1B[1m#x1B[31mE           AssertionError: timeout waiting for response status code 200 or valid data#x1B[0m

#x1B[1m#x1B[31m_integration-test/test_01_basics.py#x1B[0m:40: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Comment on lines +957 to +959
let d = &mut Deserializer::from_slice(&payload);

let mut log: LogKafkaMessage = match serde_path_to_error::deserialize(d) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Reversed the change discussed here in the original PR, our sentry-kafka-schema and the snuba consumer already expect this format so we'll have to cut another version of them to change this.

@jjbayer I know you approved but this is modifying away from your original comment, so didn't want to merge without checking. I want to try to get a single project working on s4s before doing any more schema changes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! The JSON-based message LGTM. At some point we will want to get rid of extra deserialization step here, but that's a Relay-internal refactor we will tackle later.

relay-server/src/services/store.rs Outdated Show resolved Hide resolved
Comment on lines +957 to +959
let d = &mut Deserializer::from_slice(&payload);

let mut log: LogKafkaMessage = match serde_path_to_error::deserialize(d) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! The JSON-based message LGTM. At some point we will want to get rid of extra deserialization step here, but that's a Relay-internal refactor we will tackle later.

@k-fish k-fish merged commit fbdbfe7 into master Jan 29, 2025
25 checks passed
@k-fish k-fish deleted the revert-4463-revert-4448-feat/ourlogs/ingest-log-items branch January 29, 2025 15:46
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.

2 participants