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

Fix: set "java" platform to transactions #1332

Merged
merged 5 commits into from Mar 18, 2021
Merged

Conversation

maciejwalkowiak
Copy link
Contributor

📜 Description

Fix: set "java" platform to transactions

💡 Motivation and Context

Each event/transaction that SDK sends should have platform set.

💚 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

@maciejwalkowiak
Copy link
Contributor Author

The alternative implementation that I think I would be in favour of is that SentryBaseEvent#platform is set to "java" by default when object is created. This way we could mark it as @NotNull and just let user overwrite it with non-null values if needed.

If it's not something that users should be able to overwrite we could then remove the setter completely. What do you think @bruno-garcia @marandaneto

@marandaneto
Copy link
Contributor

The alternative implementation that I think I would be in favour of is that SentryBaseEvent#platform is set to "java" by default when object is created. This way we could mark it as @NotNull and just let user overwrite it with non-null values if needed.

If it's not something that users should be able to overwrite we could then remove the setter completely. What do you think @bruno-garcia @marandaneto

better not, we also capture events from native platforms (Android NDK), it's important to know if there was a value, or not.
I think Gson would overwrite anyway but seems wrong.

@maciejwalkowiak
Copy link
Contributor Author

But do we set different value for NDK? The javadocs says:

Acceptable values are: `as3`, `c`, `cfml`, `cocoa`, `csharp`, `elixir`, `haskell`, `go`,
   * `groovy`, `java`, `javascript`, `native`, `node`, `objc`, `other`, `perl`, `php`, `python`,
   * `ruby`

@codecov-io
Copy link

Codecov Report

Merging #1332 (7c8b49b) into main (fa1b8b7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1332   +/-   ##
=========================================
  Coverage     75.64%   75.65%           
- Complexity     1842     1843    +1     
=========================================
  Files           185      185           
  Lines          6319     6321    +2     
  Branches        626      627    +1     
=========================================
+ Hits           4780     4782    +2     
  Misses         1254     1254           
  Partials        285      285           
Impacted Files Coverage Δ Complexity Δ
sentry/src/main/java/io/sentry/SentryEvent.java 69.89% <ø> (-0.95%) 42.00 <0.00> (-2.00)
...ry/src/main/java/io/sentry/MainEventProcessor.java 75.71% <100.00%> (ø) 24.00 <0.00> (ø)
...entry/src/main/java/io/sentry/SentryBaseEvent.java 84.44% <100.00%> (+1.11%) 23.00 <2.00> (+2.00)
sentry/src/main/java/io/sentry/SentryClient.java 88.09% <100.00%> (+0.08%) 83.00 <11.00> (+1.00)

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 fa1b8b7...7c8b49b. Read the comment docs.

@marandaneto
Copy link
Contributor

But do we set different value for NDK? The javadocs says:

Acceptable values are: `as3`, `c`, `cfml`, `cocoa`, `csharp`, `elixir`, `haskell`, `go`,
   * `groovy`, `java`, `javascript`, `native`, `node`, `objc`, `other`, `perl`, `php`, `python`,
   * `ruby`

NDK events are native, Dart events are other, RN events are javascript

@maciejwalkowiak
Copy link
Contributor Author

Ok, then PR is ready to review as it is.

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.

LGTM

@maciejwalkowiak maciejwalkowiak merged commit 59dfb95 into main Mar 18, 2021
@maciejwalkowiak maciejwalkowiak deleted the transactions-platform branch March 18, 2021 13:40
@bruno-garcia
Copy link
Member

But do we set different value for NDK? The javadocs says:

Acceptable values are: `as3`, `c`, `cfml`, `cocoa`, `csharp`, `elixir`, `haskell`, `go`,
   * `groovy`, `java`, `javascript`, `native`, `node`, `objc`, `other`, `perl`, `php`, `python`,
   * `ruby`

NDK will use native

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