EMB-3907 tracer provider package with android implementation #36

jpmunz · 2024-06-17T18:43:07Z

Adding a react-native-tracer-provider package. This provides a wrapper on the TracerProvider interface exposed by the (android) native SDK that also conforms to the TracerProvider interface on the JS side so that any JS otel instrumentation library should be able to use the Embrace SDK to write traces.

Keeping in draft for now as there is a number of TODOs/blockers atm:

Need to implement the iOS side of the native module, this is waiting on pointing to the latest 6.x version of the iOS Embrace SDK
Need to point to an updated version of the Android Embrace SDK that exposes the TracerProvider interface, this is under development at the moment, in the meantime I used the OTEL Java SDK for testing
During development I wrote the code and tests directly inside the 'basic-test-app', ran into a number of issues when I split this out into its own package, namely around being able to run the Kotlin tests on the native side, will need to figure out the proper setup and also make sure the package.json for this is consistent with how we've setup the other ones
Some TODOs on the JS implementation:
- pass down schemaURL to the native side correctly
- don't add a dependency on @opentelemetry/sdk-trace-web for the context manager

notion-workspace · 2024-06-17T18:43:10Z

add otel-js-api compliant interface for Embrace RN native modules (Android)

codecov · 2024-06-17T18:45:21Z

Codecov Report

Attention: Patch coverage is 94.18605% with 10 lines in your changes missing coverage. Please review.

Project coverage is 80.11%. Comparing base (e15ec11) to head (24583da).

Additional details and impacted files

@@                                     Coverage Diff                                      @@
##           jpmunz/EMBR-3907-add-trace-provider-export-package-setup      #36      +/-   ##
============================================================================================
+ Coverage                                                     77.40%   80.11%   +2.70%     
============================================================================================
  Files                                                            23       29       +6     
  Lines                                                           894     1066     +172     
  Branches                                                        169      211      +42     
============================================================================================
+ Hits                                                            692      854     +162     
- Misses                                                          202      210       +8     
- Partials                                                          0        2       +2

Files	Coverage Δ
...tracer-provider/src/EmbraceNativeTracerProvider.ts	`100.00% <100.00%> (ø)`
packages/react-native-tracer-provider/src/index.ts	`100.00% <100.00%> (ø)`
packages/react-native-tracer-provider/src/util.ts	`95.65% <95.65%> (ø)`
...ct-native-tracer-provider/src/EmbraceNativeSpan.ts	`97.67% <97.67%> (ø)`
...native-tracer-provider/src/TracerProviderModule.ts	`57.14% <57.14%> (ø)`
...-native-tracer-provider/src/EmbraceNativeTracer.ts	`88.88% <88.88%> (ø)`

... and 1 file with indirect coverage changes

alyssa-embrace · 2024-06-25T16:22:08Z

integration-tests/basic-test-app/helpers/generateSpans.ts

+
+ // Add some span events
+ span2.addEvent("test-2-event-1");
+ span2.addEvent("test-2-event-2", 1700001002000);


Do these constant values have specific meanings?

alyssa-embrace · 2024-06-25T16:26:43Z

integration-tests/basic-test-app/helpers/generateSpans.ts

+ const span1 = tracer.startSpan("test-1");
+ const span2 = tracer.startSpan("test-2", { kind: SpanKind.CLIENT });
+ const span3 = tracer.startSpan("test-3");
+ // Not ended so shouldn't be part of the output


This creates an implicit dependency where generateTestSpans needs to be called before generateNestedSpans right?

alyssa-embrace · 2024-06-25T16:28:43Z

integration-tests/specs/tracer_provider.test.ts

+describe("Tracer Provider", () => {
+ const expectValidIDs = (span: EmbraceSpanData) => {
+ expect(span.span_id).toHaveLength(16);
+ expect(span.span_id).not.toEqual("0000000000000000");


Is there an expected format for span_id and trace_id we could check for?

Oh I see. Is there any reason we can't reuse the code from expectValidIDs or am I misunderstanding?

alyssa-embrace · 2024-06-25T16:30:58Z

integration-tests/specs/tracer_provider.test.ts

+ parent_span_id: "0000000000000000",
+ name: "test-1-updated",
+ start_time_unix_nano: 1718045981827000 * 1000000,
+ end_time_unix_nano: 1718045981827000 * 1000000 + 2591701,


What are the constant added values on lines 40, 51, and 80?

alyssa-embrace · 2024-06-25T16:32:56Z

integration-tests/typings/embrace.d.ts

@@ -2,16 +2,41 @@
 // add more properties as they become relevant for test specs, see:
 // https://github.com/embrace-io/embrace-android-sdk/tree/master/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/payload

-interface SessionPayload {
+interface Session {


Payload was renamed to Message in other places. Is this correct?

alyssa-embrace · 2024-06-25T17:18:29Z

packages/react-native-tracer-provider/src/EmbraceNativeSpan.ts

+/**
+ * EmbraceNativeSpan implements a Span over the native Embrace Android and iOS SDKs.
+ *
+ * A handful of simple attributes are maintained on the JS side for each span include a unique ID based on the


I feel like this should be including instead of include

alyssa-embrace · 2024-06-25T17:24:31Z

packages/react-native-tracer-provider/src/EmbraceNativeSpan.ts

+ }
+
+ public addLink(link: Link): this {
+ return this.addLinks([link]);


Could we save an allocation on boxing the link in an array by inverting addLinks and addLink? That is, by writing this such that addLinks calls addLink rather than this way around?

alyssa-embrace · 2024-06-25T17:30:08Z

packages/react-native-tracer-provider/src/EmbraceNativeSpan.ts

+ return this.recording;
+ }
+
+ public recordException(exception: Exception, time?: TimeInput): void {


Is there a reason this one doesn't return this like some of the ones above it?

alyssa-embrace · 2024-06-25T17:45:17Z

integration-tests/basic-test-app/helpers/generateSpans.ts

+ Tracer,
+} from "@opentelemetry/api";
+
+export function generateTestSpans(tracer: Tracer) {


Random thought: is it possible to construct a span where the end-time is before the start-time?

alyssa-embrace · 2024-06-25T17:48:11Z

packages/react-native-tracer-provider/src/util.ts

+ return time;
+ }
+
+ if (time instanceof Date) {


Why are type checking semantics on 19 and 15 different?

EMB-3907 tracer provider package with android implementation

24583da

jpmunz requested a review from a team June 17, 2024 18:43

jpmunz mentioned this pull request Jun 17, 2024

EMBR-3907 package setup #35

Draft

alyssa-embrace reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EMB-3907 tracer provider package with android implementation #36

EMB-3907 tracer provider package with android implementation #36

jpmunz commented Jun 17, 2024

notion-workspace bot commented Jun 17, 2024

codecov bot commented Jun 17, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

alyssa-embrace Jun 25, 2024

EMB-3907 tracer provider package with android implementation #36

Are you sure you want to change the base?

EMB-3907 tracer provider package with android implementation #36

Conversation

jpmunz commented Jun 17, 2024

notion-workspace bot commented Jun 17, 2024

codecov bot commented Jun 17, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment