search

DataDog / expo-datadog

Datadog SDK for Expo
Apache License 2.0
8 stars 5 forks source link

Add support for custom service name #25

Closed aaronholla closed 9 months ago

aaronholla commented 9 months ago

What does this PR do?

Closes https://github.com/DataDog/expo-datadog/issues/24

Adds a new serviceName option to support overriding the service name that is used when uploading the sourcemaps.

Motivation

When uploading sourcemaps by default the Service used will be the application's bundle identifier. This does not work for projects that set a custom serviceName in the RUM configuration during runtime initialization. For datadog to un-minify error logs the Service linked to the sourcemap needs to match the serviceName used by the RUM logs.

Additional Notes

Android

~For Android, the serviceName is set in the datadog sdk config in the gradle config. Since both withAndroidProguardMappingFiles and withAndroidSourcemaps can both edit the datadog sdk config the order these plugins run in matters. Currently, because the order is determined by a hardcoded map (ERROR_TRACKING_CONFIG_PLUGINS_MAP), the order is guaranteed. However, we still have to account for withAndroidProguardMappingFiles not being run if you pass in the config androidProguardMappingFiles: false to the expo plugin.~ Android Datadog config has moved to its own plugin so we don't have conflicts in which configs are applied.

iOS

For iOS, there is an existing method of overriding the serviceName. You can also specify a SERVICE_NAME_IOS environment variable. This is documented for the underlying datadog-ci command here: https://github.com/DataDog/datadog-ci/tree/master/src/commands/react-native#customize-the-command

Review checklist (to be filled by reviewers)

aaronholla commented 9 months ago

Thanks for the feedback @louiszawadzki ❤️

I've updated the PR to account for the Android config plugin as well as the iOS cli flag position.

add the option to dSYMs upload as well

For the dSYMs I was not able to find how to pass the service name for these uploads.

I'm not seeing a Service option in the underlying cli command here https://github.com/DataDog/datadog-ci/tree/master/src/commands/dsyms

louiszawadzki commented 9 months ago

Sorry you are correct about dSYMs, we use the file UUID to match it against errors, so there is no need to specify the service in this case! I'll take some time today to review and test your PR :) By the way, if needed you can create a free Datadog Trial account here: https://www.datadoghq.com/free-datadog-trial/ :)

louiszawadzki commented 9 months ago

I think you're nearly there, you just need to sign all your commits as per our open source policy and we can merge!

aaronholla commented 9 months ago

@louiszawadzki Sorry about that. Updated all the commits to be signed.

louiszawadzki commented 9 months ago

Thanks a lot for all your work!

I will take care of the release and the documentation :)

We usually don't backport new features in the expo plugin as Expo upgrades are easier than RN upgrades, but I saw you mentioned it in the issue you opened. Do you have issues when upgrading from Expo 49 to Expo 50?

aaronholla commented 9 months ago

Thanks a lot for all your work!

I will take care of the release and the documentation :)

Thanks @louiszawadzki, happy to help out. 😃

We usually don't backport new features in the expo plugin as Expo upgrades are easier than RN upgrades, but I saw you mentioned it in the issue you opened.

Do you have issues when upgrading from Expo 49 to Expo 50?

Ah, yes. Thank you for calling this out. While I sort of agree with the approach to simplify maintenance. Expo 49 is still an officially supported sdk version by Expo. Expo tends to support three versions at a time to allow time to upgrade (currently sdks 48, 49, & 50)

For larger projects with lots of dependencies and custom expo plugins it takes a bit of time to upgrade from my experience.

This specific issue ends up being a bit of a pain to patch since this plugin went through a minor refactor from 49 to 50.

It might be useful to backport these changes to older sdk versions since this is missing functionality that makes it really hard to get proper error logs. Even worse the source maps actually upload but if you don't dig into it you may not realize the Service isn't matching.

louiszawadzki commented 9 months ago

Hi @aaronholla, I see your point.

However we have very little projects using a custom serviceName, so actually no one might benefit from this feature. As most apps use the default service value we'd rather wait to see if there is demand for backporting this before doing so.

Let me know if you need this on your app, in this case we can do the backport!

I hope this makes sense to you :)