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

Remove requirement for supplying appId #874

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented May 21, 2024

Goal

Removes the requirement for supplying appId. This will allow folks to send data from Embrace to 3rd party OTel exporters without needing to send anything to Embrace's servers.

I have taken the approach of making the DeliveryService no-op if an appId is not supplied. There is probably some additional work that we could cancel (such as construction of payload messages etc), but I feel this is a good first approach. Perhaps ultimately we can aim to export as pure OTel & therefore kill a lot of SDK code.

I would appreciate in-depth reviews on this as it substantially changes how the SDK works & I want to be sure I haven't forgotten anything important.

@fractalwrench fractalwrench requested a review from bidetofevil May 21, 2024 10:39
Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 53.65854% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 80.82%. Comparing base (53a33ef) to head (03b35cc).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #874      +/-   ##
==========================================
- Coverage   80.92%   80.82%   -0.11%     
==========================================
  Files         435      436       +1     
  Lines       11613    11629      +16     
  Branches     1764     1773       +9     
==========================================
+ Hits         9398     9399       +1     
- Misses       1437     1443       +6     
- Partials      778      787       +9     
Files Coverage Δ
...racesdk/capture/metadata/EmbraceMetadataService.kt 83.98% <ø> (ø)
...oid/embracesdk/capture/metadata/MetadataService.kt 100.00% <ø> (ø)
...ace/android/embracesdk/config/local/LocalConfig.kt 100.00% <100.00%> (ø)
...embrace/android/embracesdk/payload/NetworkEvent.kt 100.00% <100.00%> (ø)
...d/embracesdk/comms/delivery/NoopDeliveryService.kt 0.00% <0.00%> (ø)
...roid/embracesdk/config/behavior/SdkModeBehavior.kt 80.95% <0.00%> (ø)
...o/embrace/android/embracesdk/internal/BuildInfo.kt 57.89% <0.00%> (+5.51%) ⬆️
...acesdk/opentelemetry/OpenTelemetryConfiguration.kt 96.77% <0.00%> (-3.23%) ⬇️
...ace/android/embracesdk/injection/DeliveryModule.kt 81.81% <77.77%> (-18.19%) ⬇️
.../android/embracesdk/config/EmbraceConfigService.kt 91.11% <0.00%> (-2.23%) ⬇️
... and 3 more

... and 2 files with indirect coverage changes

Copy link
Collaborator

I think exporting in pure OTel is unlikely to happen in the short term given how hard it'll be to get away from needing to send session data in a single payload, but I think no-op-ing the delivery service is, as you said, a good first approach.

I think we still need to keep the payload object construction for caching reasons, but we can probably optimize what we store in that case.

@fractalwrench fractalwrench marked this pull request as ready for review May 22, 2024 09:15
@fractalwrench fractalwrench requested review from a team as code owners May 22, 2024 09:15
@fractalwrench fractalwrench changed the title [WIP] Remove requirement for supplying appId Remove requirement for supplying appId May 22, 2024
require(!appId.isNullOrEmpty()) { "Embrace AppId cannot be null or empty." }
logger.logWarning(
"No appId supplied to Embrace. This is required if you want to " +
"send data to Embrace, unless you have configured an OTel exporter."

Choose a reason for hiding this comment

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

can we detect if a user has configured an OTel exporter? if they haven't, i.e. they are sending data to our backend, it would probably be better if this threw an exception rather than issuing a warning log

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe in debug? I'm wearying about a bad SDK configuration crashing the app if we threw in prod. Like passing a null into a Kotlin String or annotated @nonnull parameter, even though what they did isn't correct, I don't think we should do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, an OTel exporter has to be configured before the SDK starts, so if we want to detect this case, we probably an.

Choose a reason for hiding this comment

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

Maybe in debug? I'm wearying about a bad SDK configuration crashing the app if we threw in prod.

yeah, fair point

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

LGTM in general. I like Fredric's suggestion of returning empty strings instead of invalid URLs - but nothing blocking from my side

@fractalwrench fractalwrench force-pushed the remove-app-id-req branch 2 times, most recently from 8b156f8 to dfee5c0 Compare May 28, 2024 09:45
@fractalwrench fractalwrench merged commit 8dcc4f3 into master May 28, 2024
4 checks passed
@fractalwrench fractalwrench deleted the remove-app-id-req branch May 28, 2024 12: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.

4 participants