DataDog / dd-sdk-reactnative

Datadog SDK for ReactNative
Apache License 2.0
121 stars 42 forks source link

Use consistent API schema between `@datadog/mobile-react-navigation` and `@datadog/browser-rum` #552

Open leggomuhgreggo opened 11 months ago

leggomuhgreggo commented 11 months ago

Is your feature request related to a problem? Please describe.

Currently there is a significant amount of divergence between the config schema + method names in @datadog/mobile-react-navigation and @datadog/browser-rum — and this creates extra implementation / maintenance overhead for users who are working on cross-platform expo apps (Web, iOS, Android).

For example: every time there's an update to any of the datadog RUM packages, where config has changed, we need to review the configuration and figure out which entries are still valid between the different RUM client packages.

Describe the solution you'd like

It would be great if the configuration schema + method names were the same between the RUM clients — and where they are not the same, it would be great if the differentiation were clearer.

I've outlined the differences here:

API Mismatch Summary **Native- vs web-specific config** - only relevant to particular platform <-- perfectly legit, but ideally distinct from possibly similar sounding options - eg `nativeCrashReportEnabled` - seemingly missing feature - eg `version` (recently added!) **Overlapping config** - same key / same value <-- this is the dream 😄 - different key / same value <-- requires some aliasing, but not too bad - eg `service` vs `serviceName` - same or slightly different key / slightly different value 🤕 - eg `firstPartyHosts` vs `allowedTracingOrigins` or `siteParameter` vs `site` - different key / same value, BUT unclear if features actually overlap 🤔 - eg `trackErrors` vs `forwardErrorsToLogs` **Misc Other Issues** - Method mismatch - names but also sync vs async - Companion libraries eg `@datadog/browser-logs` `@datadog/mobile-react-navigation` - CI / SourceMap orchestration - constructor vs init

In particular, it'd be cool to align the schema for these properties

Describe alternatives you've considered

Initially I tried to create a single abstraction for RUM client config, but I realized that the properties changed too much and weren't safe to assume as equivalent.

So now I have a subset of core properties that are shared: env service applicationId clientToken version

And for everything else I have platform-specific config objects, so my abstraction looks approximately like this:

export const initDatadog = async () => {
  await customDatadog.init(
    {
      applicationId: process.env.DD_RUM_APPLICATION_ID,
      clientToken: process.env.DD_RUM_CLIENT_TOKEN,
      env: process.env.DD_RUM_ENVIRONMENT,
      serviceName: process.env.DD_SERVICE_NAME, // RN alias of `service`
      version: process.env.APP_VERSION_DATADOG,
    },
    {
      rumNative: {
        firstPartyHosts: TRACING_ORIGINS,
        sessionSamplingRate: 90,
        resourceTracingSamplingRate: 90,
        telemetrySampleRate: 90,
      },
      rumWeb: {
        allowedTracingUrls: TRACING_ORIGINS,
        sampleRate: 90,
        sessionReplaySampleRate: 90,
        telemetrySampleRate: 90,
      },
    },
  );
};

Thanks!

louiszawadzki commented 11 months ago

Hi @leggomuhgreggo, thanks for reaching out!

This is indeed something we're aware of, we're trying to have our web and mobile APIs match as much as possible, but there are indeed some of the cases you mentioned where they differ.

Bringing full support for react-native-web is high in our backlog and will very likely improve this as we'll expose a single interface for both RN and web, I can let you know when it is ready.

I'll also raise this internally so we can see which improvements we can make in the short term.

Finally, I can maybe share some insights on some discrepancies to help you understand better the differences.

firstPartyHosts (mobile) vs allowedTracingOrigins (web): allowedTracingOrigins includes the host of your website by default, but not firstPartyHosts - since we cannot guess it on mobile. So if you copy from web to mobile, be sure to add the host of your website to your firstPartyHosts as well if that is relevant. Yet I also agree that the naming is very different when the concept is quite close, so this could be improved.

trackErrors (mobile) vs forwardErrorsToLogs (web): trackErrors enables you to report JS errors and console.error calls in React Native, to both RUM and Logs. forwardErrorsToLogs enables you to forward console.error calls in the web to Logs.

I also believe the version and site attribute is available in both browser and RN SDKs: https://docs.datadoghq.com/real_user_monitoring/browser/#configuration

I hope this clarifies a few things, let me know if you have further questions on this topic :)

leggomuhgreggo commented 11 months ago

Thank you for the thoughtful response!

Bringing full support for react-native-web is high in our backlog and will very likely improve this as we'll expose a single interface for both RN and web, I can let you know when it is ready.

Oh nice!!!

Finally, I can maybe share some insights on some discrepancies to help you understand better the differences ...I hope this clarifies a few things

Thanks for this — I didn't fully appreciate some of those nuances.

Sincerely appreciate the attention / consideration @louiszawadzki