Skip to content

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/396458573

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 2 times, most recently from 4e6a781 to 8517aa7 Compare March 12, 2025 15:56
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-metrics-otlp-json branch from 8517aa7 to a1876b4 Compare March 12, 2025 17:34
// 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
@franciscovalentecastro
Copy link
Contributor Author

All tests pass in commit d3e3f50. Rebasing and merging.

@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-metrics-otlp-json branch from d3e3f50 to bc4351b Compare March 13, 2025 21:14
@franciscovalentecastro franciscovalentecastro merged commit b72473a into master Mar 13, 2025
65 of 70 checks passed
@franciscovalentecastro franciscovalentecastro deleted the fcovalente-metrics-otlp-json branch March 13, 2025 22:15
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