-
Notifications
You must be signed in to change notification settings - Fork 34
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: tracking #268
base: main
Are you sure you want to change the base?
feat: tracking #268
Conversation
7ef16e8
to
0a8a6ff
Compare
Signed-off-by: Todd Baert <[email protected]>
0a8a6ff
to
5efd0ed
Compare
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Nicklas Lundin <[email protected]> Signed-off-by: Todd Baert <[email protected]>
|
||
### Requirement 6.2.2 | ||
|
||
> The `occurrence details` **MUST** support the inclusion of custom fields, having keys of type `string`, and values of type `boolean | string | number`. |
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.
I think that this details should be equivalent to the structure value available for contexts. Allowing not just a single-level map of raw types, but effectively any JSON representation. If it is just a flag map, then it places what seems to be an unnecessary constraint on less restrictive systems.
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.
I'm in agreement with this proposal, with the caveat that we're OK with some additional complexity in provider implementation.
While I appreciate the idea of incorporating "tracking aggregation" as a way to streamline dashboard creation, I believe it's crucial to keep the specifications clean and neutral regarding the underlying technology for data storage, combination, and aggregation. For example, there should be nothing stopping me from building a tracking provider that just outputs to a file if that's what my business requires. However, I am open to have some type of documentation of best practices or examples of use and maybe this is where such proposal belong? |
/** | ||
* Record a tracking occurrence. | ||
*/ | ||
void track(String occurrenceKey, OccurrenceDetails details, EvaluationContext context): void; |
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.
I think big use case for occurrences is joining occurrences with variants to see what "business impact" flag changes have.
The way I understand it, most people will be just "joining" this with what's proposed in Feature Flag Impression SemConv
People just need something to join both entries on. SemConv proposal only suggest logging "context id" without full context. I have been using targetingId for this use case so far. I am thinking - wouldn't it make sense to have track method schema match schema of SemConv?
I am not advocating for any particular solution, but for more alignment between those two proposals and enabling use case of joining variant impressions with occurrences.
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.
I think big use case for occurrences is joining occurrences with variants to see what "business impact" flag changes have.
The way I understand it, most people will be just "joining" this with what's proposed in Feature Flag Impression SemConv
100%. This is how I envision things would work, and how most people will use it, I think.
People just need something to join both entries on. SemConv proposal only suggest logging "context id" without full context. I have been using targetingId for this use case so far. I am thinking - wouldn't it make sense to have track method schema match schema of SemConv?
I am not advocating for any particular solution, but for more alignment between those two proposals and enabling use case of joining variant impressions with occurrences.
I struggled a lot with this same question.
I agree that generally most people would probably join on the targetingKey
but I'm not sure that would be the case 100% of the time. If we include the entire evaluation context in the track API, users have the ability to join on whatever attributes they want which are common to the telemetry data from a flag evaluation and from the track
call (perhaps an attribute called email
, for example). It's for this maximum flexibility I proposed the API take the evaluation context instead of the targetingKey
only.
Additionally, taking the entire evaluationContext
is more consistent with existing functions, keeping the API a bit more isomorphic.
All that said, I understand why taking the targetingKey
directly in the track method is appealing. Like I said, I struggled with this question for a while, and I'm open to other perspectives.
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.
💯 percent agree with what @toddbaert wrote on the flexibility aspect and I would like to add something:
Passing the whole evaluation context in the track API allows the tracking functionality to stand on its own legs as we (the OpenFeature API) provide enough data to the tracking providers to be enable slicing and dicing the produced tracking data by imaginative dimensions that could be deduced in the evaluation context.
Additionally:
We are talking about joining data between the moment of evaluation (I think of this as the moment of exposure) and the moment of event tracking. The evaluation context may have changed between these two moments in time, it can also be that these two moments are captured in services with different evaluation context.
The question is; what context is important to track?
If I could only choose one context to store, I would keep the context of the tracking event but of course, having both of these stored would for sure be the best. So I'm arguing that from a business perspective the context data from the event is more important than the context data from the evaluation.
Now there may be some nuances in the above reasoning between static- and dynamic context paradigms, especially when it comes to the moment of exposure in the static paradigm. I would argue that for the static paradigm it is not the flag evaluation (when the client bulk fetches the flags from the backend) but rather the flag access that should be counted as the moment of exposure.
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
812dabc
to
ee902e3
Compare
client.track("visited-promo-page"); | ||
|
||
// example tracking occurrence recording that a subject performed an action associated with a business goal, with the occurrence details having a particular numeric value | ||
client.track("clicked-checkout", new OccurrenceDetails(99.77)): void; |
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.
I see the spec below requires the ability to set both a Value
+ custom fields. Once we figured out the implementation details for OccurrenceDetails it would be nice to include an example that shows how to do both.
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.
Good point, I've added this:
new OccurrenceDetails(99.77).add("currencyCode", "USD")
Which is probably how it will end up looking if implemented in Java, using our existing Structure
objects.
Signed-off-by: Todd Baert <[email protected]>
This PR is a first pass at a relatively simple tracking API. The main purpose is to close what is more or less the last gap between the OpenFeature API and that of most SDKs (many SDKs which we currently support have some kind of simple "track" method analogous to what's proposed here). Preceding discussion can be found here.
Some notable choices:
occurrenceKey
/context
/occurrenceDetails
.track
method should never block but instead queue any async work; we agreed there's no reason an application author would ever want to await a I/O associated with a singletrack
call; most providers will batch these, I assumevalue
a "first class" property of the otherwise free-formOccurrenceDetails
type, so it can be easily mapped into the equivalent field of various providers (something liketargetingKey
)