-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add Honeycomb.OpenTelemetry.Autoinstrumentations #270
Conversation
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.
Looks fine to me, only queries are around the changes to the sln and adding AspNet(Core) instrumentation.
return | ||
builder | ||
#if NET6_0_OR_GREATER | ||
.AddAspNetCoreInstrumentation(options => |
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.
Could use AddAspNetCoreInstrumentationWithBaggage
from our AspNetCore package so you don't need to configure the options directly.
options.EnrichWithBaggage(); | ||
}) | ||
.AddGrpcClientInstrumentation() | ||
#endif |
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.
Should this be an else if - otherwise we could add both AspNet and AspNetCore instrumentation
EndProject | ||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Honeycomb.OpenTelemetry.Instrumentation.AspNetCore", "src\Honeycomb.OpenTelemetry.Instrumentation.AspNetCore\Honeycomb.OpenTelemetry.Instrumentation.AspNetCore.csproj", "{DB772676-A314-44DC-8EBD-971FBE6953B9}" | ||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Honeycomb.OpenTelemetry.Instrumentation.AspNetCore", "src\Honeycomb.OpenTelemetry.Instrumentation.AspNetCore\Honeycomb.OpenTelemetry.Instrumentation.AspNetCore.csproj", "{DB772676-A314-44DC-8EBD-971FBE6953B9}" |
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.
Weird - I think I'd added this to the sln 🤷🏻
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.
Oh, you did. But visual studio felt there needed to be a different GUID. 😆
MinimumVisualStudioVersion = 10.0.40219.1 | ||
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{3E7EDB35-637A-402C-ABA6-EF8E4C55C051}" | ||
EndProject | ||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Honeycomb.OpenTelemetry", "src\Honeycomb.OpenTelemetry\Honeycomb.OpenTelemetry.csproj", "{061494E1-D5D7-479E-BE88-FE0E2F94D9F5}" | ||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Honeycomb.OpenTelemetry", "src\Honeycomb.OpenTelemetry\Honeycomb.OpenTelemetry.csproj", "{061494E1-D5D7-479E-BE88-FE0E2F94D9F5}" |
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 come the project GUIDs have changed? I think this would break the lower sections.
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.
So this won't actually break the lower sections, since it's a project type GUID. These are things that visual studio uses for things (like detecting if you have the right components installed) but it's not a part of the build.
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.
Looks great, thanks @cartermp 🎉
This 'n's a draft for now since this is kind of a proof of concept (although making it "prod ready" isn't much more work).
Basically, this is like
getNodeAutoInstrumentations
, but for our package! It pulls in a bunch of instrumentations for common things in the .NET ecosystem and lets you use 'em all in one.The idea is that if you just want an easy button to add all the things, this is that button. If you care about your dependency graph and/or app footprint, or have a need to configure each instrumentation library yourself, this is not for you. Instead, you'd just add only in the instrumentation libraries that you need, optionally configuring them.
I also noticed an extraneous project reference in the AspNetCore package that isn't needed.