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

chore: add .net version in agent header #167

Merged
merged 4 commits into from
Sep 20, 2022
Merged

Conversation

poppoerika
Copy link
Contributor

.NET version was already included in agent header, but I updated the agent string from csharp: to dotnet: and version API.
I added a chart, SDK Agent/Version, on the bottom of this dashboard: https://app.lightstep.com/momento-preprod/dashboard/mr2-overview-do-not-touch/bb7bWC2V?time_window=minutes_60&sort=sdk_type_version.desc
You can observe cutomer_id = [email protected] and sdk_type_version = dotnet:6.0.9

Closes #156

@malandis
Copy link
Contributor

malandis commented Sep 20, 2022

We need to track two versions: (1) SDK assembly version number and (2) .NET runtime version. I believe the previous commit was tracking the SDK version number (1). We still need to do that in addition to what you've added here, which addresses (2).

pgautier404
pgautier404 previously approved these changes Sep 20, 2022
@pgautier404 pgautier404 self-requested a review September 20, 2022 18:37
@poppoerika
Copy link
Contributor Author

@malandis
Ahh, you're right. What we had before was our SDK version (assembly version) and what we have now is .NET Core version (6.0.9).
If I remember correctly, we first added this header to keep track of SDK version (not language or framework version).
So other SDKs only have SDK version in their headers.
I don't mind including .NET Core version in addition to SDK version but I think we want to be consistent throughout the SDKs.
cc @eaddingtonwhite @cprice404 @eaddingtonwhite

@cprice404
Copy link
Contributor

@poppoerika yes, @malandis is right, we want both. And you are right that we will want to make this consistent across the SDKs but we can tackle that after we get dotnet SDK to 1.0.

So I think we need to make a call on whether we add this as a second header, or just append it to the value string for the existing header. I can start a thread in engineering about that.

@poppoerika
Copy link
Contributor Author

poppoerika commented Sep 20, 2022

As per the conversations here, I added the second header Runtime_Version to the header (indicating .NET/.NET Core version) and also kept Agent header as it was (indicating SDK version).

MR2 would be updated afterwards to process the second header.

Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@poppoerika poppoerika merged commit ef90032 into main Sep 20, 2022
@poppoerika poppoerika deleted the chore/add-version-to-header branch September 20, 2022 22:41
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.

include .net version in agent header
5 participants