Allow specifying Collector's own Resource in the config #5402

tigrannajaryan · 2022-05-19T22:59:15Z

Resolves #5398

This adds a sub-section in the service.telemetry config section where any Resource attributes can be specified. All specified attributes will included as own metrics labels.

If service.instance.id is not specified it will be auto-generated. Previously service.instance.id was always auto-generated, so the default of the new behavior matches the old behavior.

For example we can have the following in the config:

service:
  telemetry:
    resource:
      service.instance.id: 01G3EN4NW306AFVGQT5ZYC0GEK
      service.namespace: onlineshop
      deployment.environment: production

codecov · 2022-05-19T23:03:48Z

Codecov Report

Merging #5402 (dc41385) into main (9f15964) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head dc41385 differs from pull request most recent head a5bd0fd. Consider uploading reports for the commit a5bd0fd to get more accurate results

@@            Coverage Diff             @@
##             main    #5402      +/-   ##
==========================================
+ Coverage   90.98%   91.01%   +0.02%     
==========================================
  Files         191      190       -1     
  Lines       11429    11442      +13     
==========================================
+ Hits        10399    10414      +15     
+ Misses        811      809       -2     
  Partials      219      219

Impacted Files	Coverage Δ
service/telemetry.go	`78.28% <100.00%> (+2.20%)`	⬆️
config/configmap.go	`90.09% <0.00%> (ø)`
config/mapconverter/expandmapconverter/expand.go	`100.00% <0.00%> (ø)`
...rter/overwritepropertiesmapconverter/properties.go	`100.00% <0.00%> (ø)`
config/mapconverter.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f15964...a5bd0fd. Read the comment docs.

config/moved_service.go

service/telemetry.go

bogdandrutu · 2022-05-19T23:20:52Z

service/telemetry.go

+	var instanceID string
+	instanceID = resource[semconv.AttributeServiceInstanceID]


Allow users to decide to not have this... or have it empty if they want?

instanceID, ok := resource[semconv.AttributeServiceInstanceID] if !ok { }

Empty label values don't work with OpenCensus Prometheus exporter (I didn't dig deep into why, but I tried and the label is just ignored).

Perhaps we can make "empty" string a marker for deleting the label? I.e. you can do this to stop emitting the auto-generated service.instance.id:

service: telemetry: resource: service.instance.id:

Is this valuable?

Done. You can now provide empty string to suppress built-in attributes.

@open-telemetry/collector-approvers I would like opinions on this. Is this a) valuable and b) not-surprising behaviour?

Empty strings are valid on resource attributes so it does feel a bit like surprising behavior to me.

I don't have a good replacement for this though, only thing I can think of is maybe just service.instance.id: null? since null attributes are not valid.

Yeah, null is probably a better choice. Let me try that.

service/telemetry.go

tigrannajaryan · 2022-05-24T13:53:13Z

@open-telemetry/collector-approvers please review.

TylerHelmuth · 2022-05-24T15:02:12Z

service/telemetry.go

+	// Empty Resource attributes are an indication by the user to suppress and stop
+	// emitting the particular attribute.


Are we making an assumption of how users set attributes or adding a feature that allows users to suppress attributes when they want? If it is the latter, a config option specifically for "stop emitting these attributes" might be clearer. If this approach is kept we should document it clearly.

adding a feature that allows users to suppress attributes when they want

To suppress attributes that are otherwise automatically included.

If this approach is kept we should document it clearly.

It is documented in the code in the ServiceTelemetry struct.

TylerHelmuth · 2022-05-24T15:04:28Z

This is a great addition, can the new feature be documented specifically on opentelemetry.io? There is a section of telemetry in the collector config doc. It doesn't list every config option, but I think adding this feature to the explicitly mentioned options would be good.

tigrannajaryan · 2022-05-24T15:53:00Z

This is a great addition, can the new feature be documented specifically on opentelemetry.io? There is a section of telemetry in the collector config doc. It doesn't list every config option, but I think adding this feature to the explicitly mentioned options would be good.

I think we need to look into using https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/cmd/configschema to generate the documentation and see if we can push it to opentelemetry.io (or pull from there). I don't think it is sustainable to document things here in the code and also on the website and keep the docs in sync manually. We can look into it in a separate PR, that's out of the scope for this PR.

service/telemetry.go

Resolves open-telemetry#5398 This adds a sub-section in the `service.telemetry` config section where any Resource attributes can be specified. For example we can have the following in the config: service: telemetry: resource: service.instance.id: 01G3EN4NW306AFVGQT5ZYC0GEK service.namespace: onlineshop deployment.environment: production

tigrannajaryan · 2022-05-26T00:19:16Z

@bogdandrutu @mx-psi ok to merge?

bogdandrutu · 2022-05-26T20:27:04Z

config/moved_service.go

@@ -38,6 +38,13 @@ type Service struct {
 type ServiceTelemetry struct {
 	Logs    ServiceTelemetryLogs    `mapstructure:"logs"`
 	Metrics ServiceTelemetryMetrics `mapstructure:"metrics"`
+
+	// Resource specifies user-defined attributes to include with all emitted telemetry.
+	// For metrics the attributes become Prometheus labels.


This is not 100% true, only if the exporter is prometheus.

Maybe I can just delete this line to void misleading?

mx-psi · 2022-05-27T07:47:42Z

service/telemetry.go

+		if _, ok := resource[semconv.AttributeServiceVersion]; !ok {
+			// AttributeServiceVersion is not specified in the config. Use the actual
+			// build version.
+			telAttrs[semconv.AttributeServiceVersion] = version.Version


On non-core distros, this will show latest, right? We probably want to explicitly pass a version on the Collector settings to avoid this

I haven't verified, but I think you are right. This PR didn't change the behavior.

You are right, I missed that this was not introduced by this PR. I will make a note to reproduce and open an issue then. We probably want to get rid of version.Version entirely and leave passing the version to the builder instead.

There is another semi-related issue: for Agent Management the Supervisor needs a way to fetch the Collector's version.

If we are going to change how versions are embedded in the Collector I want to use the opportunity to see if we can make it fetchable by the Supervisor. A few options:

Have a manifest file with version number recorded in it and include the manifest in Collector's installation packages.

Try to fetch version number from executable directly (as a symbol?). Not very desirable since it requires platform specific code.

Read from CLI by using --version flag (not very reliable since the output format is not precisely defined).

Something else?

I submitted a separate issue for this #5438

tigrannajaryan requested review from a team and dmitryax May 19, 2022 22:59

tigrannajaryan force-pushed the feature/tigran/resource-in-config branch from 1e74451 to 39c367d Compare May 19, 2022 23:06

bogdandrutu reviewed May 19, 2022

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/resource-in-config branch from ab6d9d5 to 9e1bb3b Compare May 20, 2022 14:00

TylerHelmuth reviewed May 24, 2022

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/resource-in-config branch from b915b7e to 4638360 Compare May 24, 2022 19:07

dmitryax reviewed May 24, 2022

View reviewed changes

service/telemetry.go Outdated Show resolved Hide resolved

tigrannajaryan requested review from mx-psi, dmitryax and bogdandrutu May 24, 2022 20:22

dmitryax approved these changes May 24, 2022

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/resource-in-config branch from dc41385 to a5bd0fd Compare May 25, 2022 00:26

tigrannajaryan merged commit 87abc21 into open-telemetry:main May 26, 2022

tigrannajaryan deleted the feature/tigran/resource-in-config branch May 26, 2022 19:41

bogdandrutu reviewed May 26, 2022

View reviewed changes

mx-psi reviewed May 27, 2022

View reviewed changes

tigrannajaryan mentioned this pull request Feb 28, 2023

[cmd/opampsupervisor] Add OpAMP supervisor skeleton open-telemetry/opentelemetry-collector-contrib#19143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying Collector's own Resource in the config #5402

Allow specifying Collector's own Resource in the config #5402

tigrannajaryan commented May 19, 2022 •

edited

Loading

codecov bot commented May 19, 2022 •

edited

Loading

bogdandrutu May 19, 2022

tigrannajaryan May 20, 2022

tigrannajaryan May 20, 2022

tigrannajaryan May 20, 2022

mx-psi May 24, 2022

tigrannajaryan May 24, 2022

tigrannajaryan commented May 24, 2022

TylerHelmuth May 24, 2022

tigrannajaryan May 24, 2022

TylerHelmuth commented May 24, 2022

tigrannajaryan commented May 24, 2022

tigrannajaryan commented May 26, 2022

bogdandrutu May 26, 2022

tigrannajaryan May 27, 2022

mx-psi May 27, 2022

tigrannajaryan May 27, 2022

mx-psi May 27, 2022

tigrannajaryan May 27, 2022

tigrannajaryan May 27, 2022

		var instanceID string
		instanceID = resource[semconv.AttributeServiceInstanceID]

		// Empty Resource attributes are an indication by the user to suppress and stop
		// emitting the particular attribute.

Allow specifying Collector's own Resource in the config #5402

Allow specifying Collector's own Resource in the config #5402

Conversation

tigrannajaryan commented May 19, 2022 • edited Loading

codecov bot commented May 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerHelmuth commented May 24, 2022

tigrannajaryan commented May 24, 2022

tigrannajaryan commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented May 19, 2022 •

edited

Loading

codecov bot commented May 19, 2022 •

edited

Loading