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

Method LaunchDarkly.init blows up when the user provides an invalid key #288

Open
jpinho opened this issue Jan 29, 2024 · 7 comments
Open

Comments

@jpinho
Copy link

jpinho commented Jan 29, 2024

Describe the bug
The method LaunchDarkly.init blows up when the user provides an invalid key.

To reproduce
Call LaunchDarkly.init with an invalid key.

Expected behavior
As a FF SDK I would expect the SDK to never blow up. Instead the init method should return a client with a failure state, that can be check unattended. Example below:

const client = LaunchDarkly.init()

client.initialized() => false
client.getStatus() => { initialized: false, initError: 'invalid SDK key', connectionEstablished: true }

I can provide a contribution, if we have an agreement here.

Logs

    error: [LaunchDarkly] Authentication failed. Double check your SDK key.
    error: [LaunchDarkly] Received error 401 (invalid SDK key) for streaming request - giving up permanently

SDK version

"launchdarkly-node-server-sdk": "^7.0.3",

Language version, developer tools

  • node 18

OS/platform

  • macOS
@kinyoklion
Copy link
Member

Hello @jpinho,

To clarify here, you don't mean an invalid SDK, but calling the init function without any parameters?

If you provide parameters, but they are invalid, the behavior is different. If you provide a key, but it is empty, then it would not encounter this situation. This is specifically the only condition under which the SDK will throw an error such as this during initialization (at least very intentionally). (Unless you are offline as well, in which case it will continue). It typically represents a programming error that we want to communicate quickly.

I know I reminded on the other issue, but this is not the current repository. Especially if you are configuring a new project you should use the 9.x version. @launchdarkly/node-server-sdk.

Thank you,
Ryan

@jpinho
Copy link
Author

jpinho commented Feb 14, 2024

Hey @kinyoklion thanks for your answer and for suggesting me the right SDK! I got it from your website, see this link and the image attached: https://launchdarkly.com/feature-flags-node-js

Screenshot 2024-02-14 at 12 15 52

Regarding the initialisation, I'm not convinced, I see your point of wanting to fail fast and giving immediate feedback to the user. But specially being this a FF client which is never mission critical, throwing runtime errors is very unsafe. See the example below:

    darklyClientInstance = initDarkly(config.LAUNCHDARKLY_SDK_KEY, {
      timeout: LAUNCH_DARKLY_CONN_TIMEOUT_SECONDS,
      logger: Log,
    });

In the case above, I'm passing a config.LAUNCHDARKLY_SDK_KEY, due to an error loading the SDK Key config, I may pass you undefined. Therefore, your SDK has no way to know if I'm actually passing something that evals to undefined or if I'm not passing the argument at all.

Now imagine a critical service running in production that fails to load a FF SDK key and fails live customer request... that doesn't look good. Of course, resiliency can be done on the service side, but this for us Launch Darkly customers means we have to run custom code to add a bunch of resiliency when dealing with LaunchDarkly client. And now multiply this effort for +10 or more micro-services... suddenly we need a shared-library 🙈 . If LD client could be just super resilient and never throw exception and instead log errors that would be way nicer.

@kinyoklion
Copy link
Member

The general thought is if you don't provide a key, then it can never work, and we should let you know. This example here does assume that you are going to define your key.

If you really want to allow that behavior, than it would be:

process.env.LAUNCHDARKLY_SDK_KEY ?? "invalid"

This will never throw because you are providing a key, even if you are intentionally providing a bad one. You will later get a 401 and we will log that, and subsequently evaluations will always report the default variation.

You don't have to provide a valid key, but you do have to provide something in order to not hit that condition.

@kinyoklion
Copy link
Member

For this issue it requires some internal discussion. Generally speaking configuration errors we have treated as usage errors. Basically code that isn't correct and will never become correct on its own. For instance not including the SDK key is something you may encounter during dev, but once it was in production wouldn't stop at runtime.

I see your case is a little different and does merit discussion.

@jpinho
Copy link
Author

jpinho commented Feb 14, 2024

Shall I move these issues under the proper repo @kinyoklion?

@kinyoklion
Copy link
Member

That would be great. Thank you!

@jpinho
Copy link
Author

jpinho commented Feb 14, 2024 via email

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

No branches or pull requests

2 participants