ReactNativeAction DataSource #829
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @nelsitoPuglisi and the rest of your teammates on |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #829 +/- ##
==========================================
- Coverage 81.45% 81.37% -0.09%
==========================================
Files 426 427 +1
Lines 11352 11391 +39
Branches 1687 1691 +4
==========================================
+ Hits 9247 9269 +22
- Misses 1399 1413 +14
- Partials 706 709 +3
|
| @@ -85,6 +85,8 @@ internal sealed class EmbType(type: String, subtype: String?) : TelemetryType { | |||
| val embAndroidReactNativeCrashJsException = EmbraceAttributeKey("android.react_native_crash.js_exception") | |||
| } | |||
|
|
|||
| internal object ReactNativeAction : System("rn_action", true) | |||
There was a problem hiding this comment.
Should all React Native telemetry go into the same name space? So this becomes react_native.action or something?
| name: String, | ||
| outcome: String, | ||
| payloadSize: Int, | ||
| properties: Map<String?, Any?>, |
There was a problem hiding this comment.
Can we just make this Map<String, String> since that's what we require anyway?
There was a problem hiding this comment.
We need "Any" in the public API. This place is the proper one to convert the properties to "schema attributes" to the expected format.
| @@ -211,6 +213,18 @@ internal class DataSourceModuleImpl( | |||
| ) | |||
| } | |||
|
|
|||
| override val rnActionDataSource: DataSourceState<RnActionDataSource> by dataSourceState { | |||
There was a problem hiding this comment.
Should we only create this data source if the framework is react native or is it assumed that any platform can log one of these?
Come to think of it, maybe we should think about moving this to the RN layer - not now, but it feels weird to have to do this in the Android/iOS layer now we know this is just a Span.
There was a problem hiding this comment.
I agree that we can move this to another layer. But for now I don't see a problem. This is created only after being called, and it's only used by the internal interface.
86e79c9
to
13fc2ee
Compare
13fc2ee
to
87f5e3a
Compare
Goal
The purpose is to capture ReactNative actions as spans instead of breadcrumbs.
Testing
Integration and Manual tests