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

fix: simplify isObfuscated messaging #253

Merged
merged 4 commits into from
Mar 19, 2025
Merged

fix: simplify isObfuscated messaging #253

merged 4 commits into from
Mar 19, 2025

Conversation

typotter
Copy link
Collaborator

@typotter typotter commented Mar 19, 2025

Motivation and Context

The deprecation of isObfuscated led to the desire to emit a warning when the SDK may be acting out of expected vis a vis obfuscated state of the config. The implementation left a lot to be desired and had some bugs.

Changes

  • Emit an info level log when setting isObfuscated
  • Drop all isObfuscatedCache and warning on mismatch
  • bump patch version
  • Also fix EppoClientParameters to have new overrides field and restore use of EppoClientParameters type in constructor. This was broken in feat: add local override functionality #184

@typotter typotter requested a review from rasendubi March 19, 2025 14:54
Copy link
Collaborator

@rasendubi rasendubi left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this 🙌

this.expectObfuscated = isObfuscated;

if (isObfuscated !== undefined) {
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: given this log is infrequent, I think warn is justified

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had warn here originally. Then I figured that the developer would see a warning at build time if they're using the deprecated methods/params. Changing this to a warn would essentially produce a redundant warning to the devs, and possibly annoy their log monitors until we release the new version with the field&method removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think that warn is OK here as otherwise nobody is going to see it 🙂 But I don't feel very strongly about it, so feel free to disagree and ignore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm. let's do warn and we'll revisit if a customer experiences issues with the volume of warnings (which we'll hopefully obviate but cutting a new major release)

@typotter typotter enabled auto-merge (squash) March 19, 2025 20:09
@typotter typotter merged commit f5080f6 into main Mar 19, 2025
8 checks passed
@typotter typotter deleted the tp/obfuscated-warning branch March 19, 2025 20:10
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.

None yet

2 participants