feat: impact metrics

[kwasniew]

Description

Adds full impact metrics support to this SDK

This PR is the first example of impact metrics implemented mostly in Yggdrasil, with the SDK doing the wrapper work.
The actual change is <100 LOC. Most of the PR is README and tests.

What the SDK has to do is:

• orchestrate calls to the Ygg engine
• add label resolution for impact metrics
• verify everything works together in a test

Interesting decisions:

• Node restores regular Unleash bucket metrics on failure, Python doesn't. This is pre-existing behavior. I didn't want to change that. However, we have smart restoration for impact metrics that is identical to the Node SDK's impact metric restoration on failure. Please shout if you disagree with this decision :)
• The ImpactMetrics class is a wrapper around the Ygg engine with extra label resolution added on top
• For testing, I decided to use file bootstrap to simplify setup and avoid mocks. I also test using the Unleash Client API and check what was sent over the wire. This test gives high ROI without testing the internals. The only thing I didn't like—but I found similar examples in other tests—is how we trigger metric sending. The public API doesn't expose an explicit send metrics method, so I use the internal method for metric sending (aggregate_and_send_metrics). I still prefer it to an arbitrary wait for the scheduler.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

• Unit tests
• Spec Tests
• Integration tests / Manual Tests

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[coveralls]

Pull Request Test Coverage Report for Build 21248587395

Details

• 65 of 70 (92.86%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.07%) to 93.342%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
UnleashClient/init.py	6	7	85.71%
UnleashClient/environment_resolver.py	14	15	93.33%
UnleashClient/periodic_tasks/send_metrics.py	8	11	72.73%

Totals
Change from base Build 19699333839:	-0.07%
Covered Lines:	701
Relevant Lines:	751

---

💛 - Coveralls

[sighphyre]

Yeah cool, this shaped up really nicely

[sighphyre]

If I remember the binding code, this can potentially raise an exception. Think that's pretty unlikely but how do you feel about making this flow not bork metrics if this fails?

[kwasniew]

hmm, I though it should never happen. But to be safe with some weird internal errors I can wrap it with an error handler

[sighphyre]

We don't want to do what we do in Node and get this from the token? I'm going to deprecate this environment across the SDKs over the next few months, just a heads up

[kwasniew]

That's a good point. I remember fixing it in Node so we can do the same here.

[sighphyre]

Ah, found the Java developer! So I don't think this adds anything to our lives in the current shape. This is necessary pattern in Java but the magic of Python properties is that we can do this later if we ever feel the need without impacting the public API. I would just expose the field directly to be honest

[kwasniew]

Honestly I didn't want to do this but got inspired by def connection_id(self) which should probably be a regular property

[sighphyre]

It probably should. Dunno why it's like that haha, probably AI code that got a bit too excited

[sighphyre]

Thank you!

[sighphyre]

I don't think this is wrong but it does look like "Java developer slipped, fell and landed in the Python runtime" to me. What you have is clear so I'm going to make some recommendations based on what I would do given my Python background and you can pick and choose some or none of this

I think the creation of the dict here is a bit unfortunate. We close over the environment and appName fields but literally only use them here to new this dict up. Smells like a way to work around Python's lack of destructuring. Good news, Python does have destructuring though!

This is also definitely begging to be a list comprehension with destructuring. That may or may not read okay to someone without a Python background but it reads fine to me

This is how I would have done this:

def __init__(self, engine: UnleashEngine, app_name: str, environment: str):
    self._engine = engine
    self._base_labels = {
        "appName": app_name,
        "environment": environment,
    }


def _variant_label(self, flag_name: str, ctx) -> str:
    variant = self._engine.get_variant(flag_name, ctx)
    if variant and variant.enabled:
        return variant.name
    if variant and variant.feature_enabled:
        return "enabled"
    return "disabled"


def _resolve_labels(
    self, flag_context: Optional[MetricFlagContext]
) -> Dict[str, str]:
    if not flag_context:
    	## Just a lil defensive copying so we don't leak mutable state
        return dict(self._base_labels)

    return {
        **self._base_labels,
        **{
            flag: self._variant_label(flag, flag_context.context)
            for flag in flag_context.flag_names
        },
    }

[kwasniew]

yup I like your version more. thanks for pointing it towards more idiomatic python

[sighphyre]

Getting some mixed messages from this field. The class is a context but so is this. Is it contexts all the way down?

Weirdly enough because of this, I have no context for what this actually represents

[kwasniew]

this is consistent with what we called it in Node SDK. This is our user context so that we can evaluate flag_names to enabled/disabled/variant_name with a user context. I agree it may be confusing here since in Node Context has a proper shape:

export interface Context {
  [key: string]: string | Date | undefined | number | Properties;
  currentTime?: Date;
  userId?: string;
  sessionId?: string;
  remoteAddress?: string;
  environment?: string;
  appName?: string;
  properties?: Properties;
}

[kwasniew]

I think this SDK uses plain dict for user context everywhere and I don't want to make a big change in this PR

[sighphyre]

Ah! It's the actual context. Yep, makes sense, let's keep it the same. Context needs a proper type some day, today is not that day