-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Updating Java options with missing defaults #15217
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Bundle ReportChanges will decrease total bundle size by 15 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-server-cjsAssets Changed:
view changes for bundle: sentry-docs-client-array-pushAssets Changed:
|
</SdkOption> | ||
|
||
<SdkOption name="maxRequestBodySize" type="enum"> | ||
<SdkOption name="maxRequestBodySize" type="enum" defaultValue="never"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's RequestSize.NONE
in the code.
Not sure if we want to reflect that or we would rather keep a perhaps more nicer to read never
, up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with lower case still, but is none
the proper customer-facing value that should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the value should be none
, not never
in this case.
Same reasoning as for the other comment applies to the casing.
</SdkOption> | ||
|
||
<SdkOption name="diagnosticLevel" type="enum"> | ||
<SdkOption name="diagnosticLevel" type="enum" defaultValue="debug"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, technically in the code it's SentryLevel.DEBUG
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave the casing alone, unless you think it would break without explicitly suggesting all caps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my doubt was more about, do we want this to reflect the code exactly or should this be more of an indication of what to pass.
If you wanted this to be strict you would have to make it SentryLevel.DEBUG
or something, but I think just debug
is fine and indicates what to pass even if it's not always strictly correct.
If you're configuring this through code you will clearly see in your editor that you need to pass SentryLevel.DEBUG
.
If you use other configuration methods we have in java (Spring's application.properties
) then it's fine to specify it as sentry.diagnostic-level=debug
as we convert the value to upper case when reading it 👍
On a side note @sfanahata we might want to figure out how to deal with the options in the Spring/Spring Boot cases as well. |
DESCRIBE YOUR PR
Adding more details to Java options since moving everything to SDKoption formatting
Preview: https://sentry-docs-git-java-options-updates.sentry.dev/platforms/java/configuration/options/
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes: