-
Notifications
You must be signed in to change notification settings - Fork 13
Annotate config with comments #52
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
base: main
Are you sure you want to change the base?
Conversation
This PR annotates our config with comments explaining what each of the components do. This is so a user can understand why we have configured each element, and understand the implications of removing a given component.
| - otelcol_googlecloudmonitoring_point_count | ||
|
|
||
| # The batch processor is in place to regulate both the number of requests | ||
| # being made and the size of those requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloud Monitoring recommends using a batch size of 200, which is the maximum batch size allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in b4875a4
| # The resourcedetection processor is configured to detect GCP resources. | ||
| # Resource attributes that represent the GCP resource the collector is | ||
| # running on will be attached to all telemetry that goes through this | ||
| # processor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource detection is required, otherwise you will write time series from many resources into the same time series in GCP and will get a lot of errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 27ae370
config/collector.yaml
Outdated
| user_agent: Google-Cloud-OTLP manifests:0.2.0 OpenTelemetry Collector Built By Google/0.121.0 (linux/amd64) | ||
|
|
||
| # The googlemanagedprometheus exporter will send metrics to | ||
| # Google Managed Service for Prometheus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google Cloud Managed Service for Prometheus. Unless you have a specific reason to do otherwise, we strongly recommend you send OTel metric data with the googlemanagedprometheus exporter to get the best query experience with the lowest cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 27ae370
| # | ||
| # Docs: | ||
| # https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/googlemanagedprometheusexporter | ||
| googlemanagedprometheus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need the IntToDouble flag somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is turned on as part of the Google-Built OpenTelemetry Collector image's ENTRYPOINT. The way these manifests interact with the container as well as all other documented sources, the flag will always be on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should the information about intToDouble flag be mentioned here to help guide in case of issues due to that flag ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 9099496
aabmass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psx95 maybe can take a second look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might require make generate to have these changes reflect into the manifests in k8s/ directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9099496
| # | ||
| # Docs: | ||
| # https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/googlemanagedprometheusexporter | ||
| googlemanagedprometheus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should the information about intToDouble flag be mentioned here to help guide in case of issues due to that flag ?
|
I'm not sure why the fixture test failed. The failure indicates that something went wrong with spinning up the cluster, but I'm not sure how to navigate these test failures. Is there somewhere else deeper I'd need to look for those failures? |
This might be because the formatting of the generated |
Fixes #51
This PR annotates our config with comments explaining what each of the components do. This is so a user can understand why we have configured each element, and understand the implications of removing a given component.