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: ✨ Add skipOptionsValidation option and validate options #77

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

pkanal
Copy link
Contributor

@pkanal pkanal commented Feb 15, 2024

Which problem is this PR solving?

Adds validation warnings for options.
Adds skipOptionsValidation option to config. This option is used when sending directly to a collector so warnings for not providing an apiKey can be suppressed.

Short description of the changes

  • Warns if there is an apiKey missing
  • Warns if there is a serviceName missing
  • Warns if a dataset is provided but the apiKey is a E&S key and a `serviceName is provided
  • Warns if dataset is missing if a Classic apiKey is provided
  • Warns if sampler has been overridden
  • Adds skipOptionsValidation option that doesn't show any of these warnings if set to true, set to false by default

How to verify that this has the expected result

  • Run the example app and don't provide an apiKey, you should see the apiKey warning and so forth
  • Run the example app and set skipOptionsValidation to true, you should see the skip options validation message and no other warnings
  • Tests pass

@pkanal pkanal force-pushed the purvi/skip-options-validation branch from 2c574b4 to 0b0d926 Compare February 15, 2024 17:39
@pkanal pkanal changed the title feat: ✨ Add skipOptionsValidation option feat: ✨ Add skipOptionsValidation option and validate options Feb 15, 2024
@pkanal pkanal self-assigned this Feb 15, 2024
@pkanal pkanal added version: bump minor A PR that adds behavior, but is backwards-compatible. type: enhancement New feature or request labels Feb 15, 2024
'🔨 WARN: Default deterministic sampler has been overridden. Honeycomb requires a resource attribute called SampleRate to properly show weighted values. Non-deterministic sampleRate could lead to missing spans in Honeycomb. See our docs for more details. https://docs.honeycomb.io/getting-data-in/opentelemetry/node-distro/#sampling-without-the-honeycomb-sdk';

export const validateOptionsWarnings = (options?: HoneycombOptions) => {
if (options?.skipOptionsValidation) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to do the logic of skipping validation inside the function instead at the top level in the SDK because it meant we could keep the SKIPPING_OPTIONS_VALIDATION_MSG contained to this file

src/validate-options.ts Outdated Show resolved Hide resolved
@@ -24,12 +24,14 @@ describe('when debug is set to true', () => {
new HoneycombWebSDK({
debug: true,
});
expect(consoleSpy.mock.calls[1][0]).toContain(
'Honeycomb Web SDK Debug Mode Enabled',
expect(consoleSpy).toHaveBeenNthCalledWith(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a lil refactor here to use Jest functions instead of array lookups because I found it really hard to pinpoint the right message quickly with the arrays 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I love this!

@pkanal pkanal marked this pull request as ready for review February 15, 2024 17:50
@pkanal pkanal requested review from a team as code owners February 15, 2024 17:50
@pkanal pkanal added this to the Beta milestone Feb 15, 2024
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

This all works great, though I do feel like there is a lot of intertwining between this and the debug file. Both things are intended to help warn a dev about incorrect setup. I left a few notes, though not sure about the broader question about how best to merge or separate this functionality from debug.

As an example, here is the output if debug mode is enabled, while setting an unnecessary dataset and missing a service name. Maybe some of that redundancy is fine though, especially with important missing items or if you want to skipValidation but still know in debug mode. What do you think?

consolewarn and debug

src/validate-options.ts Outdated Show resolved Hide resolved
src/validate-options.ts Outdated Show resolved Hide resolved
src/validate-options.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

skipit

};

export const MISSING_API_KEY_ERROR = `❌ @honeycombio/opentelemetry-web: Missing API Key. Set \`apiKey\` in HoneycombOptions. Telemetry will not be exported.`;
export const MISSING_SERVICE_NAME_ERROR = `❌ @honeycombio/opentelemetry-web: Missing Service Name. Set \`serviceName\` in HoneycombOptions. Defaulting to '${defaultOptions.serviceName}'`;
export const createHoneycombSDKLogMessage = (message: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

🤩

@pkanal pkanal merged commit 6a1fb5a into main Feb 16, 2024
8 checks passed
@pkanal pkanal deleted the purvi/skip-options-validation branch February 16, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] Add skipOptionsValidation or similar for silencing warnings
2 participants