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

Read MAX_VALUE_LENGTH from client options (#2121) #2171

Conversation

puittenbroek
Copy link
Contributor

@puittenbroek puittenbroek commented Jun 14, 2023

Hard to jump through some hoops to get the new client options for max_string_length to where it was needed.

Still kept a fallback in place since I'm unsure if client options is always available/passed a long.

Closes #2121

@puittenbroek puittenbroek force-pushed the max-string-length-from-client-options-2121 branch from c60e773 to 34c1a5f Compare June 16, 2023 12:32
@puittenbroek
Copy link
Contributor Author

@antonpirker do you have time to review this? Or someone else from your team?

@sentrivana
Copy link
Contributor

Hey @puittenbroek, thanks a lot for this PR! We're currently evaluating internally what the potential impact of this change would be with regards to the ingestion pipeline -- we'll get back to you soon.

@sentrivana
Copy link
Contributor

@puittenbroek I ran the pipeline in the meantime -- could you please take a look at the failing workflows?

I'll squeeze a review in sometime next week unless Anton beats me to it. We'll also need docs (source) for this where we explicitly warn users that bumping the string limit might result in event payloads getting too big and events getting dropped, but I can take care of writing that.

@puittenbroek
Copy link
Contributor Author

The circular import in apidocs is a PITA, because the consts file imports from sentry_sdk.integrations import Integration which itself also imports which need the consts file. Only way around that is putting my new constant at the top of the file.

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Have some questions. In general it looks really good!

tests/test_exceptiongroup.py Outdated Show resolved Hide resolved
tests/test_serializer.py Outdated Show resolved Hide resolved
tests/test_client.py Show resolved Hide resolved
sentry_sdk/client.py Outdated Show resolved Hide resolved
@antonpirker
Copy link
Member

Great work @puittenbroek !
I have left a couple of questions/suggestions.
We then have to fix eventually failing tests and add the new option to the docs repo.

@antonpirker
Copy link
Member

Added docs PR here: getsentry/sentry-docs#7465

@antonpirker
Copy link
Member

I just discovered that we already have an option "max_value_length" in the PHP sdk, so I will rename the option to be in line with the other SDK.

@antonpirker antonpirker changed the title Read MAX_STRING_LENGTH from client options (#2121) Read MAX_VALUE_LENGTH from client options (#2121) Jul 19, 2023
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

lgtm

@puittenbroek
Copy link
Contributor Author

I just discovered that we already have an option "max_value_length" in the PHP sdk, so I will rename the option to be in line with the other SDK.

Makes sense :) And thanks for the docs PR! pretty swamped at work so didn't have much time to pick this up further.

@antonpirker
Copy link
Member

No problem @puittenbroek !
Thanks again for the contribution, I guess we are now ready to merge this!

@antonpirker antonpirker merged commit 5478df2 into getsentry:master Jul 19, 2023
245 of 246 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Introduce config for MAX_STRING_LENGTH
4 participants