[Feature Request] Caller SDK details in ExtraQueryParams

[bgavrilMS]

Problem statement

Goal

Higher level SDKs provide a hint on their identity: id + version

Dev Experience

request.WithExtraQueryParameters({{ "caller-sdk-id", "41"}, { "caller-sdk-ver", "1.42.0"}}

Note: pls improve WithExtraQueryParameters so that it can be used multiple times, in which case values are appended.

Implementation

The data should be sent to ESTS via the msal-curr header and also captured in the OTEL metrics.

[bgavrilMS]

CC @localden and @jmprieur

[localden]

@bgavrilMS @neha-bhargava for the current schema, does it mean that arbitrary caller SDK IDs and versions can be included? I am assuming that we're not actually going to use 41 but rather something like azure-identity-go?

[neha-bhargava]

@localden the caller-sdk-id is the api id used to call MSAL from the higher level sdk. I think it will be some kind of numeric or alpha numeric, so yes something like 41 can be a valid value. Since this data will be added to server side telemetry, this value cannot be huge. @bgavrilMS can confirm my understanding.

[localden]

@localden the caller-sdk-id is the api id used to call MSAL from the higher level sdk. I think it will be some kind of numeric or alpha numeric, so yes something like 41 can be a valid value. Since this data will be added to server side telemetry, this value cannot be huge. @bgavrilMS can confirm my understanding.

OK so I think that's the interesting part that we need to flesh out - what are the constraints for this field? Because I can totally see someone putting their entire user agent here 😁

[TimHannMSFT]

In other internal libraries we own, we solve a very similar problem using something the caller can configure while setting up their host. I can share more details, we call it supplemental_version_info there. Thanks to @GeoK for pointing out this similarity.

[rayluo]

Problem statement

Goal

Higher level SDKs provide a hint on their identity: id + version

Dev Experience

request.WithExtraQueryParameters({{ "caller-sdk-id", "41"}, { "caller-sdk-ver", "1.42.0"}}

Note: pls improve WithExtraQueryParameters so that it can be used multiple times, in which case values are appended.

Implementation

The data should be sent to ESTS via the msal-curr header and also captured in the OTEL metrics.

@localden and @neha-bhargava also pointed out the similarity between this "higher level SDK's identity" and the existing x-app-name, x-app-version headers. Shall we consider simply having the msal-curr telemetry include x-app-name and x-app-version while keeping their current API? Besides, the WithExtraQueryParameters() is already a wild card, but using "...QueryParameter..." to mean some new http header seems going a little too far.

[localden]

@localden and @neha-bhargava also pointed out the similarity between this "higher level SDK's identity" and the existing x-app-name, x-app-version headers. Shall we consider simply having the msal-curr telemetry include x-app-name and x-app-version while keeping their current API? Besides, the WithExtraQueryParameters() is already a wild card, but using "...QueryParameter..." to mean some new http header seems going a little too far.

Yeah, overloading WithExtraQueryParameters to say that we're amending headers seems like a less intuitive option for consuming developers 😁 Or are we saying that "query parameters" are more akin to "authentication query settings" rather than literal "URI query parameters"?

[bgavrilMS]

The problem with the existing x-app-name and x-app-version headers is that they are not recorded by ESTS. And at the same time, MSALs have public APIs (I recommend deprecating them). So there are apps out there using these already and it might get messy if we start recording them.

WithExtraQueryParameters is already being used by public client folks to enable 1p-only features like MSA-PT. It's just a property bag and works well for simple settings.

Thoughts?

[neha-bhargava]

Maybe add a new API .WithCallerSdkDetails(string apiId, string version) or .WithParameters(Dictionary<string, string> parameters) instead of using ExtraQueryParameters or anything else. I think using WithExtraQueryParameters indicates that these are being sent as query parameters.

[localden]

@neha-bhargava @bgavrilMS - I don't think we need a dedicate API specifically for API ID and version, but a generic WithParameters seems like a better option. Then again, I also can see that WithExtraQueryParameters is a good choice if the use is limited and there are not a lot of scenarios where that can be abused. How would we differentiate what needs to be a query param vs. header?

[rayluo]

The term "query parameter" specifically means parameters in an http url. In OIDC, not only a request may have query parameter and "in-body" parameter, but also the typical interactive flow has more than one http urls involved. So, it was not a good choice to use "QueryParameter" to mean a generic property bag.

Besides, I do not see a problem to use a dedicate API specifically for its intended purpose, especially when we happen to already have that API.

The problem with the existing x-app-name and x-app-version headers is that they are not recorded by ESTS. And at the same time, MSALs have public APIs (I recommend deprecating them).

The x-app-name and x-app-version were meant to be recorded by ESTS (based on our API docs). If they are not actually recorded by ESTS, that simply means they have been a no-op all this time, therefore we can repurpose it now without backward compatibility concern. We can reuse the existing API, but change the underlying implementation to send the data pair into msal-curr header (and drop the x-app-name and x-app-version header).

So there are apps out there using these already and it might get messy if we start recording them.

There shouldn't be many apps out there using a currently-no-op "AppName" and "AppVersion" API. Regardless, it is a legit concern of apps sending extra-long string (maliciously?). So, we may choose to add a hardcoded size limit to those input, and then we shall be fine.

[bgavrilMS]

This is a fair point. We already have WithClientName and WithClientVersion in the SDK, but they have been deprecated. We can bring them back. We can keep the EditorBrowsable = Never so as to not pollute the public API.

Ultimately, this is supposed to be used by higher level SDKs only. There are under 10 of these.

@localden - up to you, I don't feel strongly either way.

I recommend that we force the 2 strings to be limited in size (maybe 5 chars for name and 10 chars for version?)

[localden]

Love this @bgavrilMS - if we already have the APIs, let's bring them back and make them non-browsable. Let's keep 10 chars for both the version and name.

[bgavrilMS]

For version, it should be longer, to account for things like 1.20.3-preview4. Maybe do smth like ver.substring(0, max(len(ver), 20))