-
Notifications
You must be signed in to change notification settings - Fork 73
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
FeatureManagement 4.0.0-preview2 release notes update #896
Conversation
Split files and updated based off feedback on previous PR #886 |
I don't recall where we landed on what the github release notes should look like now that these are split. Here's the current release notes: https://github.com/microsoft/FeatureManagement-Dotnet/releases I suppose we can connect all of the changes from all of the files like this: 4.0.0-preview2Microsoft.FeatureManagement UpdatesEnhancements
Microsoft.FeatureManagement.AspNetCoreEnhancements
Microsoft.FeatureManagement.Telemetry.ApplicationInsightsEnhancements
Microsoft.FeatureManagement.Telemetry.ApplicationInsights.AspNetCoreEnhancements
|
|
||
### Enhancements | ||
|
||
* Added a property to published telemetry named `TargetingId`. This is a more reliable way to connect evaluated feature flags to other telemetry events for a given target. ([#409](https://github.com/microsoft/FeatureManagement-Dotnet/issues/409)) |
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 hope this explains why a change can benefit a customer.
Added a TargetingId
property to the feature evaluation events sent to Application Insights. The TargetingId
is the identifier of a targeted user during feature evaluation. This new property allows you to correlate feature evaluation events with other telemetry data your application sends to Application Insights, as long as they share the same TargetingId
.
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.
That can work- but the nuance to me is
allows you to correlate feature evaluation events with other telemetry data your application sends to Application Insights, as long as they share the same TargetingId.
Correlating events was already possible in the previous version using one of Application Insights user id fields.
I think this change is more about decoupling the id from Application Insights / not locking ourselves to users / ensuring we emit whatever the value of "Targeting.UserId" was instead of relying on an ambient field that might not correlate with the Targeting value.
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.
Maybe we could call out that it's explicitly the targeting information from evaluation. Like:
based on the targeting information from feature evaluation.
Which says that it's no longer some ambient field- but the actual targeting information used during evaluation.
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.
in the previous version using one of Application Insights user id fields.
Sounds like a breaking change.
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.
Shouldn't be, correlating via user id is still possible.
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.
Correlating events was already possible in the previous version using one of Application Insights user id fields.
If we did this (add userid to "one of Application Insights user id fields") before and not anymore in the new release. It sounds like a breaking change to me.
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.
Yeah that would be, but we don't add it. App Insights package automatically generates and adds user id. In our early examples, we were adding AuthenticatedUserId, but it was added/managed in the customer code and not in the library. That approach would still work today.
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.
we were adding AuthenticatedUserId, but it was added/managed in the customer code and not in the library
Got it. This means we didn't provide a solution out of the box before and now we do. Customers must do it in their application code before this release. Yes, correlating via user id was already possible, but for the sake of the argument :), the whole feature management was possible by user code completely. The release notes are about what we provide out-of-the box regardless of whether something was possible or not by the user code.
I think this change is more about decoupling the id from Application Insights / not locking ourselves to users...
This is true from our perspective. From the customer perspective, it's all about correlating events. How to do it? It's an implementation detail.
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.
True, but just to note again. Application Insights generates UserId
out-of-the-box which can be used to correlate without custom customer code. Its limitation is hard refreshing or switching apps wouldn't track the user. Our example used AuthenticatedUserId
because we wanted to get around that. We could say TargetingId
is the new out-of-the-box way to get around it.
A customer today who wants to run experiments with anonymous data from users would likely still use the UserId
field (or manually set TargetingId
to the value of UserId
).
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.
Updated to your original suggestion:
Added a TargetingId property to the feature evaluation events sent to Application Insights. The TargetingId is the identifier of a targeted user during feature evaluation. This new property allows you to correlate feature evaluation events with other telemetry data your application sends to Application Insights, as long as they share the same TargetingId.
releaseNotes/Microsoft.Featuremanagement.Telemetry.ApplicationInsights.AspNetCore.md
Outdated
Show resolved
Hide resolved
|
||
### Breaking Changes | ||
|
||
No breaking changes in this release. |
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.
How strongly do we feel the necessity of adding this no-breaking-change disclaimer? We don't do this in other release notes. Even if we put this, there is no guarantee of no breaking changes. There are just no known breaking changes.
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'll remove it for now- seeing as the guidelines don't suggest we have it.
Fixes misspelling and updates a bullet of the release notes.