Skip to content
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

Merged
merged 10 commits into from
Oct 7, 2024
Merged

feat: tracking #268

merged 10 commits into from
Oct 7, 2024

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Aug 26, 2024

Adds tracking


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:

  • I've used the term "occurrence" instead of "event" to minimize terminology overloading
  • I've ordered the parameters for the tracking method occurrenceKey/context/occurrenceDetails.
  • I've specified the 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 single track call; most providers will batch these, I assume
  • I've made value a "first class" property of the otherwise free-form OccurrenceDetails type, so it can be easily mapped into the equivalent field of various providers (something like targetingKey)

specification/types.md Outdated Show resolved Hide resolved
specification/sections/06-tracking.md Show resolved Hide resolved
@toddbaert
Copy link
Member Author

Draft JS implementation available here: open-feature/js-sdk#1020

@toddbaert
Copy link
Member Author

toddbaert commented Sep 30, 2024

@moredip suggests we perhaps use something other than "occurrence" (see comment here in draft JS implementation).

I went with "occurrence" to reduce collision with our existing event concepts (for example, we already have event and eventDetails objects), but we can disambiguate these other ways:

Currently I've proposed:

void track(String occurrenceKey, EvaluationContext context, OccurrenceDetails details): void;

but an obvious alternative would be:

void track(String trackingEventKey, EvaluationContext context, TrackingEventDetails details): void;

I'm also not sure if trackingEventId or trackingEventKey is better; we use key for flags themselves, but a key to me suggests a string index a specific set (perfect for flags in flag-sets) whereas an id might be a "softer" reference, such as a UUID.

Does anyone have points in favor of one or the other?

@toddbaert
Copy link
Member Author

Demo of some of these features: https://www.youtube.com/watch?v=Od-7Pjs2dKQ&t=436s

@roncohen
Copy link
Contributor

Does anyone have points in favor of one or the other?

The occurrence terminology threw me off initially. I understand the background for picking occurrence over event/trackingEvent, but I feel that in the context of the track call, the confusion should be manageable and the advantage of aligning on known terminology is substantial.

So I’m in favor of going with trackingEvent, trackEvent or simply event (and trackingEventKey, trackEventKey or eventKey respectively)

I'm also not sure if trackingEventId or trackingEventKey is better

Agreed that “key” is better here. Some platforms give specific events UUIDs for deduplication purposes etc which is not what we’re talking about here. I think “name” could also be a contender. Segment refers to this as the name (e.g. “event name”): https://segment.com/docs/connections/spec/track/

@roncohen
Copy link
Contributor

roncohen commented Oct 1, 2024

In the interest of moving this forward, I'd go for:

void track(String trackingEventName, EvaluationContext context, TrackingEventDetails details): void;

@toddbaert
Copy link
Member Author

In the interest of moving this forward, I'd go for:

void track(String trackingEventName, EvaluationContext context, TrackingEventDetails details): void;

I like this and based on slack we have a consensus. Will update.

@toddbaert
Copy link
Member Author

toddbaert commented Oct 1, 2024

I've made updates based on the most recent feedback, and rebased on main. As usual with spec PRs, we want a wide consensus; so far I think I've seen that represented in this PR and in various meetings and communications in Slack.

I will leave this PR open for additional comments, but I plan to merge it early next week. If you have comments or concerns, please voice them here before then! 🙏

@roncohen
Copy link
Contributor

roncohen commented Oct 2, 2024

Nice @toddbaert. Looks like there's still some "occurrence" terminology left in this PR.

@toddbaert
Copy link
Member Author

Nice @toddbaert. Looks like there's still some "occurrence" terminology left in this PR.

Nice catch, @roncohen . I think I got them all this time 😅 . There's still a few cases of the word "occurrence" but I think they are simply descriptive, not params/types.

toddbaert and others added 10 commits October 7, 2024 13:39
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Nicklas Lundin <nicklaslundin@gmail.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert merged commit cd99c35 into main Oct 7, 2024
7 checks passed
@toddbaert toddbaert deleted the feat/tracking branch October 7, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants