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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set SDK version on Transactions. #1314

Merged
merged 7 commits into from
Mar 10, 2021
Merged

Set SDK version on Transactions. #1314

merged 7 commits into from
Mar 10, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

馃摐 Description

Set SDK version on Transactions.

馃挕 Motivation and Context

Fixes #1307

馃挌 How did you test it?

Unit tests.

馃摑 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

val transaction = SentryTransaction("name", "op")
val sdkVersion = SdkVersion("transaction.sdk.name", "version")
transaction.sdk = sdkVersion
transaction.environment = "transactionEnvironment"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
transaction.environment = "transactionEnvironment"

Comment on lines 849 to 858
fixture.sentryOptions.sdkVersion = SdkVersion("sdk.name", "version")
val sut = fixture.getSut()
val transaction = SentryTransaction("name", "op")
sut.captureTransaction(transaction)
assertEquals(fixture.sentryOptions.sdkVersion, transaction.sdk)
}

@Test
fun `when transaction has SDK version set, and the SDK version is set on options, options values are not applied to transactions`() {
fixture.sentryOptions.sdkVersion = SdkVersion("sdk.name", "version")
Copy link
Contributor

Choose a reason for hiding this comment

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

could we set SdkVersion already in the creation of sentryOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll refactor this whole test class in subsequent PR.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left a comment about testing, other than that, LGTM

@codecov-io
Copy link

Codecov Report

Merging #1314 (447703c) into main (87afadb) will increase coverage by 0.19%.
The diff coverage is 93.02%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1314      +/-   ##
============================================
+ Coverage     75.65%   75.85%   +0.19%     
- Complexity     1786     1798      +12     
============================================
  Files           183      183              
  Lines          6207     6229      +22     
  Branches        622      623       +1     
============================================
+ Hits           4696     4725      +29     
+ Misses         1232     1226       -6     
+ Partials        279      278       -1     
Impacted Files Coverage 螖 Complexity 螖
sentry/src/main/java/io/sentry/NoOpSpan.java 33.33% <50.00%> (+22.22%) 6.00 <1.00> (+4.00)
...entry/src/main/java/io/sentry/NoOpTransaction.java 21.42% <50.00%> (+3.57%) 6.00 <1.00> (+1.00)
sentry/src/main/java/io/sentry/SpanStatus.java 97.05% <50.00%> (-2.95%) 10.00 <1.00> (+1.00) 猬囷笍
sentry/src/main/java/io/sentry/Hub.java 75.65% <100.00%> (+0.14%) 77.00 <2.00> (+1.00)
sentry/src/main/java/io/sentry/Scope.java 93.30% <100.00%> (+0.26%) 69.00 <3.00> (+1.00)
sentry/src/main/java/io/sentry/SentryClient.java 88.54% <100.00%> (+0.08%) 80.00 <0.00> (+1.00)
...try/src/main/java/io/sentry/SentryTransaction.java 100.00% <100.00%> (+1.63%) 35.00 <7.00> (+2.00)
sentry/src/main/java/io/sentry/Span.java 96.00% <100.00%> (+0.54%) 13.00 <6.00> (酶)
...src/main/java/io/sentry/transport/RateLimiter.java 78.30% <0.00%> (+0.94%) 28.00% <0.00%> (+1.00%)
.../main/java/io/sentry/transport/HttpConnection.java 79.59% <0.00%> (+1.02%) 21.00% <0.00%> (酶%)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 7c3f88a...337f854. Read the comment docs.

@maciejwalkowiak maciejwalkowiak merged commit 6611960 into main Mar 10, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1307 branch March 10, 2021 14:38
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.

SentryBaseEvent.sdk should be populated for transactions as well
3 participants