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

self_metrics : Write enabled_receivers and feature_tracking metrics to OTLP json. #1882

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

franciscovalentecastro
Copy link
Contributor

@franciscovalentecastro franciscovalentecastro commented Feb 11, 2025

Description

Write enabled_receivers and feature_tracking metrics to OTLP json. This PR doesn't remove the current metric collection from the diagnostics service.

Related issue

b/333412970

How has this been tested?

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@franciscovalentecastro franciscovalentecastro changed the title [Draft] Write enabled_receivers and feature_tracking metrics to OTLP json. self_metrics : Write enabled_receivers and feature_tracking metrics to OTLP json. Feb 11, 2025
@franciscovalentecastro franciscovalentecastro marked this pull request as ready for review February 11, 2025 21:36
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-metrics-otlp-json branch from f1e1485 to 63ebf59 Compare March 11, 2025 22:25
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-metrics-otlp-json branch from 8517aa7 to a1876b4 Compare March 12, 2025 17:34
runHealthChecks()
log.Println("Startup checks finished")
if *healthChecks {
// If healthchecks is set, stop here
return nil
}
case "otel":
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does does run in this case? Or does it not matter (i.e. we only want this run once). Adding a comment here would help clarify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, the new generated otlp metric files (feature_tracking_otlp.json, enabled_receivers_otlp.json) are somehow part of the Otel Collector configuration, a pre-condition for the Ops Agent otel collector to work as expected and collect self metrics about it's configuration. Considering this i made the decision to place the files in the same /run folder as the otel.yaml config :

"/run/google-cloud-ops-agent-opentelemetry-collector/otel.yaml"
"/run/google-cloud-ops-agent-opentelemetry-collector/feature_tracking_otlp.json"
"/run/google-cloud-ops-agent-opentelemetry-collector/enabled_receivers_otlp.json"

Why does does run in this case? Or does it not matter (i.e. we only want this run once). Adding a comment here would help clarify ?

It matters it only being in otel service (with the previous considerations) because :

  1. The main service (service == "") does not write Config Files (it only does config validation and runs health checks). The "fluentbit" and "otel" services write the Config Files.
  2. This way only "otel" service writes otel collector related config files.
  3. [General Design] The systemd (and windows) services run independently. We do set preconditions for order of execution, but (as i've explored in the past) setting expectations from one service doing something before something else happens in another always potentially could create unexpected race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.D. I tried putting everything in uc.GenerateFilesFromConfig to make everything simpler, but we need both the userConfig and the mergedConfig.

filepath.Join(os.Getenv("PROGRAMDATA"), dataDirectory, "run"),
filepath.Join(s.outDirectory, subagent)); err != nil {
outDir := filepath.Join(s.outDirectory, subagent)
if subagent == "otel" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, why is this a otel specific check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded in the other comment.

@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-metrics-otlp-json branch from 81c36e5 to 276d198 Compare March 13, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants