Add lambda ESM analytics #11883
Add lambda ESM analytics #11883
Conversation
joe4dev
added
the
semver: patch
LocalStack Community integration with Pro 2 files 2 suites 1h 28m 23s ⏱️ Results for commit e28afdd. ♻️ This comment has been updated with latest results. |
joe4dev
requested review from
dominikschubert,
dfangl and
gregfurman
as code owners
|
|
||
| def record(self, key: str, value: str): | ||
| namespace = f"{self.namespace}:{key}" | ||
| set_counter = self._counters.setdefault(key, UsageSetCounter(namespace)) |
There was a problem hiding this comment.
I think using setdefault here might lead to an issue: when initializing a UsageSetCounter, it will register itself in the register with collector_registry[namespace] = self, overriding on the namespace key and the counter.
I think we should probably not create the entry if config.DISABLE_EVENTS is enabled.
edit: sorry I published as an individual comment instead of the review! 😬
I'm not sure about the analytics, if it allows nested dict, I think right now on the analytics we only have one dict: namespace -> key -> value, and now we get main_namespace -> sub_namespace -> key -> value
edit2: I think I misread it and didn't see how the namespace was created and aggregated. This actually is really good this way! Sorry for missing it! Just the registration is an issue 😄
edit3: after the sync, we realized we didn't need to aggregate this multiset, as the individual counters already register themselves.
| if namespace in self._counters: | ||
| set_counter = self._counters[namespace] | ||
| else: | ||
| set_counter = UsageSetCounter(namespace) | ||
| self._counters[namespace] = set_counter |
There was a problem hiding this comment.
This is not thread safe, we might unnecessarily instantiate some usagesetcounters with the same namespace here, which might override each other in an unprecedented way. This could for example happen in multiple event source mappings in multiple thread.
Since we cannot use setdefault, I'd rather just add a lock here.
There was a problem hiding this comment.
great catch 💯
Is it thread-safe now?
|
as discussed we've decided to postpone this for now and dive a bit deeper into the requirements first. i think it's worth delineating between usage analytics and observability, and perhaps understand more what we can learn from tracking things like errors. let's discuss post v4 :-) |
joe4dev
force-pushed
the
add-esm-pipes-analytics
branch
from
9399ec6 to
e28afdd
Compare
I extracted the new ideas into a separate PR up for discussion (at a suitable time) #11934 and reduced the changes here to our existing analytics utility (i.e., the UsageSetCounter we use in other places). This minimal version would help us track how much the data plane is used (vs. users just creating resources in the control plane) and which behavior features might need attention (indicated by a high error rate). @thrau Does this harm ATM? |
Motivation
Add usage analytics for Lambda ESM to learn more about potential errors we should tackle and usage of different event sources.
Changes
invocation(successful ones) anderror(unexpected error; i.e., LS bug)