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

Make Collector instance ID available to config.MapProvider #4272

Closed
tigrannajaryan opened this issue Oct 26, 2021 · 4 comments
Closed

Make Collector instance ID available to config.MapProvider #4272

tigrannajaryan opened this issue Oct 26, 2021 · 4 comments

Comments

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Oct 26, 2021

The Collector currently generates a random instance ID in colTelemetry.initOnce() and uses the the value as the "service.instance.id" attribute in the reported telemetry. This is good and is expected.

Unfortunately this value doesn't seem to be accessible anywhere. In particular config.MapProvider needs the instance ID to report it to the management server to fetch the configuration for the particular Collector instance. See Remote Configuration Proposal for description of how the instance id can be used.

Maybe we need to make the instance ID available to config.MapProvider by some means. Perhaps it can be a readable property of the Host and Host is passed as a parameter to config.MapProvider.Get(). Note that config.MapProvider is created very early, before colTelemetry.initOnce() is called, so it is not good enough to make instance ID a public property of the Collector. It is too late, by the time Collector is created config.MapProvider should be created already.

@bogdandrutu
Copy link
Member

I disagree with this way of thinking. We need to understand better: 1. who is "generating" the instance id? Why is this not a property configured by the user (the current instance ID was a hack added to be able to differentiate between collectors without user having to do anything). We should think about the big picture:

  1. Who is responsible to configure "resource" attributes for the collector telemetry? service name/instance/namespace etc.
  2. Are these values the same that we need in the remote config? Why are we mixing telemetry with configuration (just a question not suggesting that we should not)?

Please consider to address the overall picture instead of jumping to conclusions like that instance.id should be passed to config manager.

@tigrannajaryan
Copy link
Member Author

All good questions. This indeed requires more thinking. For now I am just trying to capture the problems that I notice.

Why is this not a property configured by the user (the current instance ID was a hack added to be able to differentiate between collectors without user having to do anything).

It seems automatically generating the instance id is a good idea and matches the Otel semantic conventions. Asking the user for the instance id seems like unnecessary burden. I don't see what we gain by that.

I think we will likely want to allow the user to set the service.namespace and maybe allow to override service.name. However, I can't think of a use case where overriding the instance id is useful. Maybe there is some that I am not aware of.

Who is responsible to configure "resource" attributes for the collector telemetry? service name/instance/namespace etc.

It is done by colTelemetry currently. Probably better approaches are possible, e.g. the Collector or Service may generate the instance id and pass down to whoever needs it. This can be particularly useful if combined with a setting that overrides service.namespace and service.name, which should be probably readable from config file or command line (see identify_self_attributes here, which probably needs to be re-thought to also apply to telemetry somehow).

Are these values the same that we need in the remote config? Why are we mixing telemetry with configuration (just a question not suggesting that we should not)?

Yes, using the same value enables interesting functionality, such as linking remote configuration/management and reported telemetry. It is currently work in progress on Agent Management group but this (very long) document explains how the 2 aspects of management can work together for increased value: https://docs.google.com/document/d/1auDpvnhnWGGUlXA5y2tNXt_NVwOlgTi7aLpKm9kvFPM/edit

Please consider to address the overall picture instead of jumping to conclusions like that instance.id should be passed to config manager.

We need to somehow make the config.MapProvider and Telemetry use the same instance id. Today colTelemetry generates the instance id and sets the resource attributes. If we generate the instance id elsewhere then the solution may look different, however the final goal remains the same: the instance ID needs to be the same in config.MapProvider and for colTelemetry.

tigrannajaryan added a commit to tigrannajaryan/opamp-go that referenced this issue May 19, 2022
- The Supervisor can manage OpenTelemetry Collector.
- Demonstrates basic features: applying config, configuring Collector to collect its own metrics.

TODO:
- Find a way to fetch Collector version instead of hard-coding it.
- Set instance id in the Collector config file to make sure OpAMP and Collector
  metrics use the same instance id.
  (Related open issue open-telemetry/opentelemetry-collector#4272)
- Re-think callbacks to avoid unnecessary restarts
  (See open-telemetry#77)
- Add a way for Supervisor to understand why the Collector process exited unexpectedly.
@tigrannajaryan
Copy link
Member Author

This should be likely replaced by #5398 instead.

@bogdandrutu
Copy link
Member

#5398 is done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants