Make yace embeddable in OTel Collectors - #1837
Conversation
|
Thanks for submitting this! This is really cool. I'll plan on taking a look at this tomorrow for a thorough review. |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package otelcollector |
There was a problem hiding this comment.
WDYT about adding a godoc comment here or in a separate doc.go file as described here to convey that this package is experimental and subject to changes?
Comments on package declarations should provide general package documentation. These comments can be short, like the sort package’s brief description:
// Package sort provides primitives for sorting slices and user-defined
// collections.
package sort
They can also be detailed like the gob package’s overview. That package uses another convention for packages that need large amounts of introductory documentation: the package comment is placed in its own file, doc.go, which contains only those comments and a package clause.
There was a problem hiding this comment.
On that note anything we should add to the README or is it too early? https://github.com/prometheus-community/yet-another-cloudwatch-exporter?tab=readme-ov-file#yace---yet-another-cloudwatch-exporter
| return nil, err | ||
| } | ||
|
|
||
| interval, err := cfg.awsScrapeInterval() |
There was a problem hiding this comment.
How do you think this parameter should play with https://github.com/prometheus/opentelemetry-collector-bridge/blob/5e92765d01563b43d343bc0054c2bc65c6bb6e8e/config.go#L32?
Typically users expect would expect a default AWS scrape interval of 5 minutes/300s. Can we set the default for the AWS scrape interval for this as well?
What should happen if someone sets the AWS scrape interval to less than the configured bridge scrape interval? I would think that's not a desirable situation as there could be data loss and wasted AWS calls so perhaps we should validate against it?
| "go.opentelemetry.io/collector/receiver" | ||
| ) | ||
|
|
||
| var receiverType = component.MustNewType("yace_exporter") |
There was a problem hiding this comment.
Wondering if you think the _exporter naming has a chance of confusing users with https://opentelemetry.io/docs/collector/components/exporter/? Should we do yace_receiver instead? Or perhaps just yace? Looking at https://opentelemetry.io/docs/collector/components/receiver/ it seems a lot of them are named in the config files as a single word.
|
|
||
| registry := prometheus.NewRegistry() | ||
| for _, metric := range exporter.Metrics { | ||
| if err := registry.Register(metric); err != nil { |
There was a problem hiding this comment.
I think we have to consider tightening the scope of the YACE-wide metrics that get collected about the run health
yet-another-cloudwatch-exporter/pkg/promutil/prometheus.go
Lines 26 to 87 in cbafcec
If I configure an OTel collector process with say
receivers:
yace/prod:
config_file: /etc/yace/prod.yml
aws_scrape_interval: 300s
yace/staging:
config_file: /etc/yace/staging.yml
aws_scrape_interval: 300s
exporters:
otlphttp:
endpoint: https://otlp.example.com
service:
pipelines:
metrics:
receivers: [yace/prod, yace/staging]
exporters: [otlphttp]
Then the resulting metrics e.g.
are package-global counters. There would result two duplicate series of the sum of the two receivers counter values with no differentiation and thus I think not useful.There was a problem hiding this comment.
Oh, that's so interesting! This sounds like the classic trade-off between Push vs Pull. If we used separate exporters for prod/staging, the Prometheus scrape process would add the job/instance labels, and that would be enough to differentiate the two. Now that we're enabling push, the scrape-related labels are being lost!
I think we need to solve this in the bridge library, since this will be a problem to all exporters that are being made embeddable 🤔
There was a problem hiding this comment.
I've opened prometheus/opentelemetry-collector-bridge#27
| if _, err := c.Options(); err != nil { | ||
| return err | ||
| } | ||
| _, err := c.jobsConfig(discardLogger()) |
There was a problem hiding this comment.
I think this discards warning validation logs right? Such as this one. I think it makes sense to keep them for caller's FYI but open to thoughts.
There was a problem hiding this comment.
Hmmmm, that's true... but all these functions are also being called during Start() and in Start() we are passing a real logger.
This made me think... why do we need to call those in Validate() and also during Start()? 🤔
| func NewFactory() receiver.Factory { | ||
| return prombridge.NewFactory( | ||
| receiverType, | ||
| newLifecycleManager(slog.Default()), |
There was a problem hiding this comment.
What actually sets the default logger in this case? Like I want to configure debug level YACE logging in the OTel receiver config, how does that look and does this actually build the receiver with the logger hooked back into the collector?
d322e29 to
2dca175
Compare
| groups: | ||
| aws-sdk-v2: | ||
| patterns: | ||
| - "github.com/aws/aws-sdk-go-v2*" |
There was a problem hiding this comment.
Not sure if this applies in this module as well 🤔
There was a problem hiding this comment.
yeah AFAIK it just groups related updates together into one PR, instead of opening a PR for each individual package. Since go.mod includes many github.com/aws/aws-sdk-go-v2 packages it will help
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
4b30e91 to
8836c86
Compare
|
prometheus/opentelemetry-collector-bridge#28 was merged to address the log wiring and fc15644 implements it |
| @@ -0,0 +1,100 @@ | |||
| module github.com/prometheus-community/yet-another-cloudwatch-exporter/otelcollector | |||
There was a problem hiding this comment.
Please remove this package. There is no good reason to have to maintain multiple Go modules within Prometheus repos.
|
I'm closing this PR since we have agreed on keeping the new module outside of this repo :) |
This PR makes YACE embeddable in custom OTel Collector distributions through OCB. Implementing prometheus/proposals#69.
To facilitate this PR's review, I've opened prometheus/prometheus-opentelemetry-collector#4, which embeds YACE into a custom collector distribution. That repository's Makefile should contain everything needed to build and run the collector with YACE embedded.