-
Notifications
You must be signed in to change notification settings - Fork 274
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
[resource host+process] Allow host name and process owner attributes to be excluded #2084
[resource host+process] Allow host name and process owner attributes to be excluded #2084
Conversation
…as part of the resource
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2084 +/- ##
==========================================
+ Coverage 73.91% 77.03% +3.12%
==========================================
Files 267 5 -262
Lines 9615 135 -9480
==========================================
- Hits 7107 104 -7003
+ Misses 2508 31 -2477
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/OpenTelemetry.Resources.Host/HostResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Resources.Process/ProcessResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
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.
LGTM
Note for myself from https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/#recommended:
|
There is couple types of the resources
Host resource detector sem conv contains 7 recommended and 2 Opt-in attributes. Not all of them are implemented here. I think that we need better to expose this publicly (not on the merge level, it can be on adding method). Adding ~9 boolean attributes is not good idea IMO. Maybe we should have If operate on strings - what should happen when someone want to disable Required attribute or is passing attribute not supported by the given resource detectot? It will be great to give it also possibility to configure it from env vars to make it AutoInstrumentation friendly. |
@Kielek Personally I'm not a fan of APIs which rely on magic strings. Could we do something more strongly typed? public static class HostResourceBuilderExtensions
{
public static ResourceBuilder AddHostDetector(this ResourceBuilder builder) {}
public static ResourceBuilder AddHostDetector(this ResourceBuilder builder, Action<HostResourceOptions> configure) {}
}
public sealed class HostResourceOptions
{
internal HostResourceOptions(IConfiguration configuration)
{
var config = configuration.GetValue("OTEL_DOTNET_HOST_RESOURCE_ATTRIBUTES", (string?)null);
if (config != null)
{
// TODO: Parse config and apply onto bools (somehow)
}
}
#region Recommended (on by default)
public bool IncludeArchitecture { get; set; } = true;
public bool IncludeId { get; set; } = true;
public bool IncludeImageId { get; set; } = true;
public bool IncludeImageName { get; set; } = true;
public bool IncludeImageVersion { get; set; } = true;
public bool IncludeName { get; set; } = true;
public bool IncludeType { get; set; } = true;
#endregion
#region Opt-In (off by default)
public bool IncludeIpAddress { get; set; }
public bool IncludeMacAddress { get; set; }
#endregion
} |
@flaviocdc, could you please follow @CodeBlanch description? At least for already implemented resources? |
I agree with the idea of using configurations through options. I suggest doing the same for the remaining resources. @flaviocdc, please update comment by adding open-telemetry/opentelemetry-specification#4218 |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
Process owner and Host name may contain private data, so there has to be a way to not include them as part of the telemetry record.
In order to achieve this, I'm adding an extra parameter to
AddHostDetector
andAddProcessDetector
that allows consumers on the detectors to control whether or not this data will be added to theResource
.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes