Skip to content

Conversation

@kwasniew
Copy link
Collaborator

@kwasniew kwasniew commented Jan 23, 2026

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
Copy link

Pull Request Test Coverage Report for Build 21279450660

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 Coverage Status
Change from base Build 19699333839: -0.07%
Covered Lines: 701
Relevant Lines: 751

💛 - Coveralls

@kwasniew kwasniew closed this Jan 23, 2026
@github-project-automation github-project-automation bot moved this from New to Done in Issues and PRs Jan 23, 2026
@kwasniew kwasniew deleted the impact-metrics-with-ygg-2 branch January 23, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants