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

Add and support receiver creator endpoint properties #25866

Conversation

rmfitzpatrick
Copy link
Contributor

Description:
Adding a feature - These changes add a new boolean accept_endpoint_properties config field and mechanism to evaluate remotely specified receiver template configuration by observer endpoint environment content (disabled by default). This feature will allow the receiver creator to add or modify evaluated receiver template config, rules, and resource attributes specified by the applicable observer metadata for the containing endpoint alone.

Link to tracking Issue:
#17418

Testing:
Added and updated tests suites.

Documentation:
Updated readme to describe feature.

@dmitryax
Copy link
Member

Couple Qs for clarification:

  • What properties do I need to set to scrape metrics only from the annotated (let's say redis) pods and ignore other redis pods that are not annotated?
  • Does removing annotations stop metrics scraping?

@rmfitzpatrick
Copy link
Contributor Author

rmfitzpatrick commented Aug 17, 2023

  • What properties do I need to set to scrape metrics only from the annotated (let's say redis) pods and ignore other redis pods that are not annotated?

The rule/config/resource_attributes* will only be applied to the endpoints* that contains the properties, so it doesn't transfer to any other endpoint evaluation from an unannotated redis pod. This occurs in observerHandler.OnAdd() for each observed endpoint.

  • Does removing annotations stop metrics scraping?

Yes, since the observerHandler.OnChange() method removes the target receiver with OnRemove() and then runs OnAdd() with the updated endpoint no longer containing the annotation, its properties aren't able to be evaluated again so only the service config would apply.

@dmitryax
Copy link
Member

The rule/config/resource_attributes* will only be applied to the endpoint that contains the properties, so it doesn't transfer to any other endpoint evaluation from an unannotated redis pod. This occurs in observerHandler.OnAdd() for each observed endpoint.

If it's only applied to the endpoint containing the properties, why do we need to transfer the recievercreator's endpoint discovery configuration to the endpoint itself? I'm talking about the rule annotation. Shouldn't it be just a boolean annotation or just the presence of any other receiver-creator annotation?

@rmfitzpatrick
Copy link
Contributor Author

rmfitzpatrick commented Aug 17, 2023

If it's only applied to the endpoint containing the properties, why do we need to transfer the recievercreator's endpoint discovery configuration to the endpoint itself? I'm talking about the rule annotation. Shouldn't it be just a boolean annotation or just the presence of any other receiver-creator annotation?

For port endpoints, the annotations come from the sub-pod, so a default true would cause a hit for all ports (all container ports), when likely a single is desired. Similarly for container endpoints, an endpoint exists for each port (endpointForPort). This way specifying the port in the* type == "port" and port == xyz or type == "container" and port == xyz rules* is helpful.

edit: In cases where a receiver's default configuration leads to telemetry, just setting the rule label/annotation is all that's needed.*

@dmitryax
Copy link
Member

For port endpoints, the annotations come from the sub-pod, so a default true would cause a hit for all ports (all container ports), when likely a single is desired. Similarly for container endpoints, an endpoint exists for each port (endpointForPort). This way specifying the port in the* type == "port" and port == xyz or type == "container" and port == xyz rules* is helpful.

Shouldn't port be provided as part of config? Configuring an endpoint discovery rule on the endpoint itself seems a bit confusing.

@rmfitzpatrick
Copy link
Contributor Author

rmfitzpatrick commented Aug 21, 2023

Shouldn't port be provided as part of config? Configuring an endpoint discovery rule on the endpoint itself seems a bit confusing.

I think this may be a general receiver creator use question: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/receivercreator#rule-expressions. The port variable in an environment can be used in an arbitrary place in a value* in config, but is also part of the endpoint identity. Because there is an endpoint generated for each port, all of which share labels/annotations from an env, having the port as an identifier (being included in a rule)* acts as a filter.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 5, 2023
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorconfigfromenv branch from fa9d9b1 to ae8c22a Compare September 6, 2023 14:31
@github-actions github-actions bot removed the Stale label Sep 7, 2023
Copy link
Contributor

@hughesjj hughesjj left a comment

Choose a reason for hiding this comment

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

LGTM

@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorconfigfromenv branch from ae8c22a to 767b806 Compare September 25, 2023 13:42
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 10, 2023
@dmitryax dmitryax removed the Stale label Oct 10, 2023
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 25, 2023
@atoulme atoulme removed the Stale label Oct 25, 2023
@atoulme atoulme assigned atoulme and unassigned dashpole Oct 25, 2023
@@ -182,10 +190,10 @@ func (obs *observerHandler) OnRemove(removed []observer.Endpoint) {
}

for _, rcvr := range obs.receiversByEndpointID.Get(e.ID) {
obs.params.TelemetrySettings.Logger.Info("stopping receiver", zap.Reflect("receiver", rcvr), zap.String("endpoint_id", string(e.ID)))
obs.logger.Info("stopping receiver", zap.Reflect("receiver", rcvr), zap.String("endpoint_id", string(e.ID)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably can peel off this logger change in a separate PR just to make the diff smaller

@@ -135,3 +139,23 @@ func (cfg *Config) Unmarshal(componentParser *confmap.Conf) error {

return nil
}

func (rt receiverTemplate) copy() receiverTemplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

move it up to around line 70 to keep it close to the struct definition?

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 15, 2023
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorconfigfromenv branch from 767b806 to e38305d Compare November 29, 2023 12:29
@crobert-1 crobert-1 removed the Stale label Nov 29, 2023
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 14, 2023
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

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

Successfully merging this pull request may close these issues.

6 participants