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

feat(protocol): Add jvm debug file type #2002

Merged
merged 10 commits into from
Apr 17, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Apr 4, 2023

To support Java Source Context we add a new debug file type called jvm. More info on it can be found in getsentry/sentry-java#633 (comment)

/// The number of milliseconds Sentry spent resolving sources for a java event.
///
/// This metric is measured in Sentry and reported in the java processing task.
#[metastructure(field = "ms.processing.jvmbased")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, this is needed.

@adinauer adinauer marked this pull request as ready for review April 7, 2023 09:48
@adinauer adinauer requested a review from a team April 7, 2023 09:48
@adinauer adinauer changed the title Add new debug file type called jvmbased Add jvmbased debug file type Apr 7, 2023
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

From Relay's point of view, this looks good to go! Please do get approval also from whoever else is involved in this initiative.

@@ -486,6 +508,8 @@ pub enum DebugImage {
Wasm(Box<NativeDebugImage>),
/// Source map debug image.
SourceMap(Box<SourceMapDebugImage>),
/// JVM based debug image.
JvmBased(Box<JvmBasedDebugImage>),
Copy link
Member

Choose a reason for hiding this comment

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

I would rename JvmBased to Jvm everywhere, but that is just a personal preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah agreed, this also came up in another PR. Will rename it and update this PR.

@adinauer adinauer changed the title Add jvmbased debug file type Add jvm debug file type Apr 11, 2023
Copy link
Member

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

LGTM. Please update the PR title to match the commit message format.

CHANGELOG.md Outdated Show resolved Hide resolved
relay-general/src/protocol/debugmeta.rs Show resolved Hide resolved
relay-general/src/protocol/debugmeta.rs Outdated Show resolved Hide resolved
adinauer and others added 2 commits April 11, 2023 11:09
Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
@adinauer adinauer changed the title Add jvm debug file type feat(protocol): Add jvm debug file type Apr 11, 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 (dont know where the new metric ms_processing_jvm is measurued, but I guess it is just good to have this in place)

@adinauer adinauer merged commit 1769714 into master Apr 17, 2023
17 checks passed
@adinauer adinauer deleted the feat/new-debug-file-type-jvmbased branch April 17, 2023 06: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.

None yet

4 participants