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

feat: Allow Disabling the default Exporter #328

Merged
merged 7 commits into from
Oct 16, 2024

Conversation

arriIsHere
Copy link
Contributor

Which problem is this PR solving?

We have had a few requests come in asking for the ability to disable the default exporter. This makes it possible through a new property in the configuration.

Short description of the changes

This adds a new SDK configuration property disableDefaultTraceExporter that, when set to true will disable the default exporter.

How to verify that this has the expected result

Provide this boolean as true in an example and note no events are sent to honeycomb.

@arriIsHere arriIsHere changed the title Allow Disabling the default Exporter feat: Allow Disabling the default Exporter Oct 15, 2024
Copy link
Contributor

@MustafaHaddara MustafaHaddara left a comment

Choose a reason for hiding this comment

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

I am also not married to the name but I think this is one of those things where there is no perfect name and we could spend weeks bikeshedding about it.

The current name is Fine™️

I vote we ship it! :shipit:

Copy link
Contributor

@wolfgangcodes wolfgangcodes left a comment

Choose a reason for hiding this comment

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

This works as advertised (esp in conjunction with traceExporters option). I'm curious if it's more inline with exceptions where folks can set the default exporter like this:

const sdk = new HoneycombWebSDK({
    apiKey: 'someKey // Replace with your Honeycomb Ingest API Key
    serviceName: 'coolApp', 
    defaultExporter: MyDefaultTraceExporter(),
    traceExporters: [ExporterMcExporteryFace(), TraceExporter2()]
    ...
    }

And disable it with:

const sdk = new HoneycombWebSDK({
    apiKey: 'someKey // Replace with your Honeycomb Ingest API Key
    serviceName: 'coolApp', 
    defaultExporter: null,
    traceExporters: [ExporterMcExporteryFace(), TraceExporter2()]
    ...
    }

I wonder if @pkanal or @martin308 have any instincts here. 🤔

README.md Outdated Show resolved Hide resolved
@pkanal
Copy link
Contributor

pkanal commented Oct 16, 2024

@wolfgangcodes I'm strongly in favour of the boolean option, a null option makes me nervous about null/undefined checks and this feels really lightweight and explicit to me!

Co-authored-by: Wolfgang Therrien <[email protected]>
@arriIsHere arriIsHere marked this pull request as ready for review October 16, 2024 15:15
@arriIsHere arriIsHere requested a review from a team as a code owner October 16, 2024 15:15
@arriIsHere
Copy link
Contributor Author

Only thing I am going to change is push to unshift so that it behaves the same way it does in the previous code.

configureHoneycombHttpJsonTraceExporter(options),
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We may want a check/logger statement here so that if folks disable the default exporter AND the don't provide any other exporters, there's at least something that's like console.warn("You gots no exporters configured.")

@arriIsHere arriIsHere merged commit 31f5047 into main Oct 16, 2024
14 checks passed
@arriIsHere arriIsHere deleted the arriIsHere.disable-default-exporter branch October 16, 2024 17:39
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