-
Notifications
You must be signed in to change notification settings - Fork 7
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
Refactor how Embrace attributes are defined #526
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bidetofevil and the rest of your teammates on |
@@ -375,12 +375,10 @@ public final class io/embrace/android/embracesdk/spans/EmbraceSpanEventJsonAdapt | |||
public fun toString ()Ljava/lang/String; | |||
} | |||
|
|||
public final class io/embrace/android/embracesdk/spans/ErrorCode : java/lang/Enum, io/embrace/android/embracesdk/internal/spans/EmbraceAttributes$Attribute { | |||
public final class io/embrace/android/embracesdk/spans/ErrorCode : java/lang/Enum { |
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.
This is marked as @BetaApi so I think this change is OK, especially given that customers shouldn't be using those methods anyway.
d699687
to
a70cb16
Compare
bb0352f
to
c77be02
Compare
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.
LGTM
a70cb16
to
894c375
Compare
c77be02
to
ec36512
Compare
894c375
to
76865c0
Compare
ec36512
to
92e8c37
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #526 +/- ##
==========================================
+ Coverage 79.45% 79.59% +0.14%
==========================================
Files 403 406 +3
Lines 10861 10867 +6
Branches 1602 1601 -1
==========================================
+ Hits 8630 8650 +20
+ Misses 1585 1575 -10
+ Partials 646 642 -4
|
Merge activity
|
76865c0
to
e3c84d8
Compare
92e8c37
to
61848e4
Compare
Goal
Create an abstraction for an Embrace Attribute and define a sealed class for each attribute and an implementation of that class for each valid value.
I converted two attributes just to get some feedback as to whether this is a good idea/implementation. If it looks good, I can convert EmbType/TelemetryType to this format as well.
The goal is to be able to use these class when validating values in tests, and also when we write out the property to OTel objects, instead of having to hardcode rules like putting
emb.
in front of attribute names, etc.If this looks good, I'll add convenience methods to internal objects and tests to simply the code that words with Embrace attributes, so you can do something like span.setEmbraceAttribute(ErrorCode.Failure)
Testing
Existing tests cover the usage, but unit tests should be added to test the abstract classes if this is deemed a good way to go.