opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

`scraperhelper.NewScraper` is awkward to use

Open mx-psi opened this issue 1 year ago • 4 comments

While working on #9208 I realized that the current way scraperhelper.NewScraper works is a bit awkward. The function takes a name string and then builds an ID with it:

https://github.com/open-telemetry/opentelemetry-collector/blob/f5a7315cf88e10c0bce0166b35d9227727deaa61/receiver/scraperhelper/scraper.go#L64

It is very common among contrib scrapers to name the scraper with the same name as the component itself, and so we end up building a component.Type, converting it to string to pass to NewScraper and then building it again. I propose we change the first argument to take a component.Type instead.

mx-psi avatar Feb 06 '24 12:02 mx-psi

I think Scraper interface needs a bit more "love" than that. Will give a try to send some PRs/ideas soon

bogdandrutu avatar Feb 06 '24 17:02 bogdandrutu

I agree that it's a bit awkward to use and encountered similar, this is my "triage" attempt. Feel free to rip it apart or add to it if useful

Of note is that we may want to start limiting string length of component.Type as well

hughesjj avatar Mar 02 '24 00:03 hughesjj

So, we've change the underlying datatype from String to Struct in
9472, but there's still a few points of "awkwardness" remaining. From my perspective:

  • We don't allow "empty" Types to exist via the regex, but the empty value is still in use. This means not every constructed type will pass the validation logic.
  • ScraperHelper constructs a component.ID but never uses the Name field. This is awkward because the constructor itself takes in a string, and I've seen downstream consumers pass in component.ID.String() as the value for this. This leads to unexpected behavior given, if a Name is set on the component.ID passed into NewScraper, the .String() value will not pass the regex. Ex if a component.ID is type=foo,name=bar, the .String() method will be foo/bar and fail the regex.

hughesjj avatar Mar 06 '24 18:03 hughesjj

the empty value is still in use

Where do we use the empty value?

mx-psi avatar Mar 07 '24 11:03 mx-psi