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

Attempt to match Java distro resources #249

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

cartermp
Copy link
Member

@cartermp cartermp commented Sep 14, 2022

I think this gets us as close as we can with #119 given the following constraints:

  • Can't use the new .NET 6 APIs that give you much richer OS and runtime information
  • Can't rely on upstream OTel, since they don't have these resources implemented in the SDK
  • Can't rely on pre-defined values for runtime info from upstream since they aren't specified

That said, I think it's valuable to have something rather than nothing.

@cartermp cartermp requested review from a team and robbkidd September 14, 2022 22:00
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Other than checking we have the correct number of items in an array, looks good 👍🏻

private static ResourceBuilder AddRuntimeResource(this ResourceBuilder builder)
{
var frameworkDescription = RuntimeInformation.FrameworkDescription;
var parts = frameworkDescription.Split(' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's wise to test we have the correct number of parts or we risk a invalid index runtime exception.

Copy link
Member Author

@cartermp cartermp Sep 15, 2022

Choose a reason for hiding this comment

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

Yeah, actually it's a bit more complicated than this because of .NET Core and .NET Framework. So what we're really after is a rudimentary parser. I'll do that.

@MikeGoldsmith MikeGoldsmith added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Sep 15, 2022
@MikeGoldsmith MikeGoldsmith merged commit 40ad7a8 into main Sep 16, 2022
@MikeGoldsmith MikeGoldsmith deleted the cartermp/match-java-resources branch September 16, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants