-
Notifications
You must be signed in to change notification settings - Fork 67
feat: impact metrics #387
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: impact metrics #387
Changes from all commits
303c8c1
41adbe3
5730f0c
8b21608
a637b07
a9fa478
2ee3eb8
bca1d32
bf554e9
d4b9794
d41836b
bb4445f
a935bd3
eeb1e48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| from typing import Dict, Optional | ||
|
|
||
|
|
||
| def extract_environment_from_headers( | ||
| headers: Optional[Dict[str, str]], | ||
| ) -> Optional[str]: | ||
| if not headers: | ||
| return None | ||
|
|
||
| auth_key = next( | ||
| (key for key in headers if key.lower() == "authorization"), | ||
| None, | ||
| ) | ||
| if not auth_key: | ||
| return None | ||
|
|
||
| auth_value = headers.get(auth_key) | ||
| if not auth_value: | ||
| return None | ||
|
|
||
| _, sep, after_colon = auth_value.partition(":") | ||
| if not sep: | ||
| return None | ||
|
|
||
| environment, _, _ = after_colon.partition(".") | ||
| return environment or None |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| from dataclasses import dataclass, field | ||
| from typing import Any, Dict, List, Optional | ||
|
|
||
| from yggdrasil_engine.engine import UnleashEngine | ||
|
|
||
|
|
||
| @dataclass | ||
| class MetricFlagContext: | ||
| """Context for resolving feature flag values as metric labels.""" | ||
|
|
||
| flag_names: List[str] = field(default_factory=list) | ||
| context: Dict[str, Any] = field(default_factory=dict) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
}
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this SDK uses plain dict for user context everywhere and I don't want to make a big change in this PR
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
|
|
||
| class ImpactMetrics: | ||
| """ | ||
| Provides methods to define and record metrics (counters, gauges, histograms) | ||
| with optional feature flag context that gets resolved to labels. | ||
| """ | ||
|
|
||
| def __init__(self, engine: UnleashEngine, app_name: str, environment: str): | ||
| self._engine = engine | ||
| self._base_labels = { | ||
| "appName": app_name, | ||
| "environment": environment, | ||
| } | ||
|
|
||
| def define_counter(self, name: str, help_text: str) -> None: | ||
| self._engine.define_counter(name, help_text) | ||
|
|
||
| def increment_counter( | ||
| self, | ||
| name: str, | ||
| value: int = 1, | ||
| flag_context: Optional[MetricFlagContext] = None, | ||
| ) -> None: | ||
| labels = self._resolve_labels(flag_context) | ||
| self._engine.inc_counter(name, value, labels) | ||
|
|
||
| def define_gauge(self, name: str, help_text: str) -> None: | ||
| self._engine.define_gauge(name, help_text) | ||
|
|
||
| def update_gauge( | ||
| self, | ||
| name: str, | ||
| value: float, | ||
| flag_context: Optional[MetricFlagContext] = None, | ||
| ) -> None: | ||
| labels = self._resolve_labels(flag_context) | ||
| self._engine.set_gauge(name, value, labels) | ||
|
|
||
| def define_histogram( | ||
| self, name: str, help_text: str, buckets: Optional[List[float]] = None | ||
| ) -> None: | ||
| self._engine.define_histogram(name, help_text, buckets) | ||
|
|
||
| def observe_histogram( | ||
| self, | ||
| name: str, | ||
| value: float, | ||
| flag_context: Optional[MetricFlagContext] = None, | ||
| ) -> None: | ||
| labels = self._resolve_labels(flag_context) | ||
| self._engine.observe_histogram(name, value, labels) | ||
|
|
||
| def _variant_label(self, flag_name: str, context: Dict[str, Any]) -> str: | ||
| variant = self._engine.get_variant(flag_name, context) | ||
| if variant and variant.enabled: | ||
| return variant.name | ||
| if variant and variant.feature_enabled: | ||
| return "enabled" | ||
| return "disabled" | ||
|
|
||
| def _resolve_labels( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
},
}
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup I like your version more. thanks for pointing it towards more idiomatic python |
||
| self, flag_context: Optional[MetricFlagContext] | ||
| ) -> Dict[str, str]: | ||
| if not flag_context: | ||
| return dict(self._base_labels) | ||
|
|
||
| return { | ||
| **self._base_labels, | ||
| **{ | ||
| flag: self._variant_label(flag, flag_context.context) | ||
| for flag in flag_context.flag_names | ||
| }, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| from UnleashClient.environment_resolver import extract_environment_from_headers | ||
|
|
||
|
|
||
| def test_valid_headers(): | ||
| custom_headers = { | ||
| "Authorization": "project:environment.hash", | ||
| "Content-Type": "application/json", | ||
| } | ||
|
|
||
| result = extract_environment_from_headers(custom_headers) | ||
| assert result == "environment" | ||
|
|
||
|
|
||
| def test_case_insensitive_header_keys(): | ||
| custom_headers = { | ||
| "AUTHORIZATION": "project:environment.hash", | ||
| "Content-Type": "application/json", | ||
| } | ||
|
|
||
| result = extract_environment_from_headers(custom_headers) | ||
| assert result == "environment" | ||
|
|
||
|
|
||
| def test_authorization_header_not_present(): | ||
| result = extract_environment_from_headers({}) | ||
| assert result is None | ||
|
|
||
|
|
||
| def test_environment_part_is_empty(): | ||
| custom_headers = { | ||
| "Authorization": "project:.hash", | ||
| } | ||
|
|
||
| result = extract_environment_from_headers(custom_headers) | ||
| assert result is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!