aws-observability / aws-rum-web

Amazon CloudWatch RUM Web Client
Apache License 2.0
126 stars 66 forks source link

Provide option to disable monkey-patching of global methods and allow flexibility in configuring http clients. #161

Open gillisandrew opened 2 years ago

gillisandrew commented 2 years ago

Hey, really appreciate the efforts to simplify the initial setup experience but I'm having a couple of issues with the current implementation.

Specific Issues

In my case I would like to instrument an application with http telemetry enabled but I'm faced with a couple of different challenges.

  1. The remote service will sometimes return non-error response code that contain error objects that should be reported to RUM.
  2. The local client is built to handle certain classes of http errors (notably 403s) with a retry mechanism but there is no clean way to catch these errors and handle them appropriately.
  3. Certain error responses contain information that should be made available as annotations and metadata in X-Ray but the current implementation doesn't provide a way to enrich those fields.

Proposed Features

  1. Allow an option to disable monkey-patching of fetch and instead expose the instrumented client on the AwsRum instance.
  2. Allow a fetch API compatible function to be passed as the default http client in the CWR configuration object. Currently this could technically be achieved by monkey-patching fetch before initializing CWR web client, but that is far from ideal.
  3. Allow instrumentation of a fetch client in the local scope. This should allow application developers to instrument each service for their own requirements.
  4. Provide a mechanism for extending the annotations and metadata in an X-Ray segment.

It's not clear if you are accepting contributions, particularly those extending the public API of the web client. If you are I'd be happy to implement some of these features myself.

maniator commented 2 years ago

I've contributed some, so I do believe they are accepting

That sounds like it could be a great idea too 🙂

maniator commented 2 years ago

i have an open PR for uodatijg tyoes of monkey patch, and another for plugins (so that new things can be added by others in a much easier way)

qhanam commented 2 years ago

Hi @gillisandrew thanks for the detailed issue! Yes, we are open to contributions.

**1. Allow an option to disable monkey-patching of fetch and instead expose the instrumented client on the AwsRum instance.

  1. Allow a fetch API compatible function to be passed as the default http client in the CWR configuration object. Currently this could technically be achieved by monkey-patching fetch before initializing CWR web client, but that is far from ideal.**

It is possible to disable the FetchPlugin and XhrPlugin by excluding http from the telemetries config. For example:

const config: AwsRumConfig = {
  ...
  telemetries: ['errors', 'performance']
};

It is also possible to add a custom plugin using the config. For example:

import { AwsRum, AwsRumConfig, FetchPlugin } from 'aws-rum-web';

const config = {
    logicalServiceName: 'sample.rum.aws.amazon.com',
    urlsToInclude: [/response\.json/],
    recordAllRequests: true,
    addXRayTraceIdHeader: true
};

const config: AwsRumConfig = {
  ...
  eventPluginsToLoad: [new FetchPlugin(config)],
  telemetries: ['errors', 'performance']
};

Would this solve the use case?

Notes:

3. Allow instrumentation of a fetch client in the local scope. This should allow application developers to instrument each service for their own requirements.

Could a custom plugin support this use case as well?

4. Provide a mechanism for extending the annotations and metadata in an X-Ray segment.

Yes! This is in our backlog. Specifically, @limhjgrace is looking into it if you'd like to co-ordinate.

gillisandrew commented 2 years ago

@qhanam Thanks for the response! An xray plugin would actually really help address some of the immediate challenges. If it isn't something that is being released in the next couple of weeks I'll likely work on it as an external package just to unblock the current project.

I'd be sure to reference the issue and open a pull request once there is a working solution.

It is possible to disable the FetchPlugin and XhrPlugin by excluding http from the telemetries config

Yeah, I'm aware but we do hope to use the data collected by the FetchPlugin, we just need some flexibility in how things are handled on the client.

qhanam commented 2 years ago

Gotcha. Sounds awesome, thanks @gillisandrew !

limhjgrace commented 2 years ago

Hey @gillisandrew! The custom X-Ray annotations feature is definitely in our backlog and I'll most likely be able to get to it mid-June. However, this doesn't include extending the metadata. Could I know more about your use case? Do you have an idea of what the additional data would look like?

gillisandrew commented 2 years ago

@limhjgrace Hey thanks for working on this. I just lumped annotations together with metadata because they both add data to the traces. Annotations would be sufficient to address the current issue we're having.