Conversation
github-actions
Bot
added
the
Scope: Frontend
| ], | ||
| }); | ||
|
|
||
| export const metrics = < |
There was a problem hiding this comment.
Looks like the java platforms (java, java-log4j, java-spring, etc) are missing in the withMetricsOnboarding Set in platformCategories.tsx.
Then it shows up in Explore > Metrics.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
) <!-- Describe your PR here. --> <!-- Sentry employees and contractors can delete or ignore the following. --> ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --------- Co-authored-by: Lukas Bloder <lukas.bloder@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| export const limitedMetricsSupportPrefixes: Set<string> = new Set([ | ||
| 'android', | ||
| 'java', |
There was a problem hiding this comment.
Are we listing only java because it serves as a prefix for java-logback, java-log4j2, java-spring, and java-spring-boot?
There was a problem hiding this comment.
There seems to only be prefixes here for other SDKs so I replicated this for Java too.
| export const metricsVerify = < | ||
| PlatformOptions extends BasePlatformOptions = BasePlatformOptions, | ||
| >( | ||
| params: DocsParams<PlatformOptions> | ||
| ): ContentBlock => ({ | ||
| type: 'conditional', | ||
| condition: params.isMetricsSelected, | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: t( | ||
| 'Send test metrics from your app to verify metrics are arriving in Sentry.' | ||
| ), | ||
| }, | ||
| { | ||
| type: 'code', | ||
| tabs: [ | ||
| { | ||
| label: 'Java', | ||
| language: 'java', | ||
| code: getMetricsVerifyJavaSnippet(), | ||
| }, | ||
| { | ||
| label: 'Kotlin', | ||
| language: 'kotlin', | ||
| code: getMetricsVerifyKotlinSnippet(), | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
It appears that this function is not used anywhere. Knip may be failing in this PR, though I haven’t confirmed yet.
|
|
||
| ${getMetricsCodeJava()}`; | ||
|
|
||
| export const getMetricsVerifyKotlinSnippet = () => `import io.sentry.Sentry |
There was a problem hiding this comment.
this is only being used in the metricsVerify which is not used anywhere. we should do a cleanup in this file.
There was a problem hiding this comment.
Will clean the files up a bit.
| { | ||
| type: 'text', | ||
| text: tct( | ||
| "To start using metrics, make sure your Sentry Java SDK version is [code:8.30.0] or higher. If you're on an older version of the SDK, follow our [link:migration guide] to upgrade.", |
There was a problem hiding this comment.
nit: should we say version here?
| docsPlatform, | ||
| }: { | ||
| docsPlatform: string; |
There was a problem hiding this comment.
nit: maybe java can be the default? or do we want to force developers to always inform the platform?
There was a problem hiding this comment.
now inferring docsPlatform as you suggested
| replayOnboardingJsLoader, | ||
| feedbackOnboardingJsLoader, | ||
| logsOnboarding: logs, | ||
| metricsOnboarding: metrics({docsPlatform: 'spring'}), |
There was a problem hiding this comment.
Based on what I observed, if we remove the java- prefix, the names are identical. I propose we avoid making the metrics a function and instead use something like params.platformKey.split('java-')[1] directly.
Co-authored-by: Priscila Oliveira <priscila.oliveira@sentry.io>
|
@adinauer The issue is that the component renderWithOnboardingLayout I’ll update this test utility in a separate PR, so we don’t need to modify the JavaScript tests here in this PR. They shouldn’t fail when updating a Java platform. |
…latformKey (#106557) Because the platformKey is static - java-spring-boot - enabling Metrics for this platform is causing the JavaScript tests to fail, even though they shouldn’t ([see](#106508 (comment))).
priscilawebdev
left a comment
priscilawebdev
left a comment
There was a problem hiding this comment.
Nit: I’d update the sentence and combine the verify metrics code with the error so there’s one less code snippet ....just like we do for javascript-react:
Other than that, it looks good to me! 🚀
|
Created getsentry/sentry-java#5043 to track the follow up for combining snippets so we can get this merged. |
3 participants
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.