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

Fix returned OS name on Linux #1791

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Fix returned OS name on Linux #1791

merged 1 commit into from
Mar 10, 2023

Conversation

albertvaka
Copy link
Contributor

What does this PR do?

Fixes #1790

Additional notes

This might be a breaking change.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@albertvaka albertvaka requested a review from a team as a code owner March 9, 2023 16:40
@pr-commenter
Copy link

pr-commenter bot commented Mar 9, 2023

Benchmarks

Comparing candidate commit fc363c1 in PR branch albertvaka/fix-linux-osname with baseline commit 308285a in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@nsrip-dd nsrip-dd added this to the v1.49.0 milestone Mar 9, 2023
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

Good catch, thank you for the fix! I've confirmed that this looks good on a Linux box.

@albertvaka
Copy link
Contributor Author

This fixes the code but I wonder if this could break queries that our customers already have. Is it okay to merge or should we wait until a major version bump?

@nsrip-dd
Copy link
Contributor

nsrip-dd commented Mar 9, 2023

Good question. Right now this value is only used in the tracer & profiler's startup logs. It's not attached as a tag to traces or metrics. I mainly see those logs as providing a quick set of information for support. From that POV this patch makes those logs strictly better. IMO this change is okay. But I suppose a customer could be basing a query off of those startup logs, and could care that the "os_name" value stays the same. Hyrum's law and all that.

I'll ping somebody from the tracer team for a second opinion.

@knusbaum
Copy link
Contributor

knusbaum commented Mar 9, 2023

This is mostly useful for debugging, along with all the other things Nick said. Fixing this will definitely be helpful for customers.

@albertvaka
Copy link
Contributor Author

Okay, merging then!

@albertvaka
Copy link
Contributor Author

albertvaka commented Mar 9, 2023

Oh, it looks like I don't have permissions to merge.

@nsrip-dd nsrip-dd merged commit edce413 into main Mar 10, 2023
@nsrip-dd nsrip-dd deleted the albertvaka/fix-linux-osname branch March 10, 2023 13:17
@nsrip-dd
Copy link
Contributor

Thanks @albertvaka!

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.

[BUG] Reported OS name on Linux is always "Linux (Unknown Distribution)"
3 participants