(flow) node_exporter integration #1872
Conversation
|
|
||
| func init() { | ||
| component.Register(component.Registration{ | ||
| Name: "integration.node_exporter", |
There was a problem hiding this comment.
I think the name is fine for now, but we probably want to rethink it at some point.
What is an integration? 🤷 What are the types of things which go into the integration component group?
exporter.node or something along those lines might make more sense down the line as we add more components.
There was a problem hiding this comment.
That would require a refactor on the docs, right? we're using the term integration everywhere so I guess that integrations components should have a similar name if we dont want to create confusion to users.
There was a problem hiding this comment.
We have an opportunity to fix the naming with Flow. I felt like the integration terminology got a bit overused. Things like the app_agent_receiver probably deserves to be in its own component namespace separate from all the components which are Prometheus exporters.
Generally, I'm also willing to delay naming decisions right now until we have more components, since more components will make it easier to see the emerging pattern for how we want to name things. I did want to bring this up so that we start thinking about it, though.
There was a problem hiding this comment.
I completely agree integrations are super overloaded. I'm gonna go with exporter.node.
There was a problem hiding this comment.
#2059 suggests specifically prometheus.integration.node_exporter and explains why the "exporter" terminology isn't recommended (since it collides with OpenTelemetry's usage of the term)
We can discuss what the final name should be, but I think it should be scoped in the prometheus namespace at least.
| return | ||
| } | ||
| // remove /component/{id} from front of path, so each component can handle paths from their own root path | ||
| r.URL.Path = strings.TrimPrefix(r.URL.Path, "/component/"+id) |
There was a problem hiding this comment.
Can we put a helper method in the component package to get a path prefix by component ID? It's being spread across two packages (here and the component) and it feels a little magic-stringy.
There was a problem hiding this comment.
Yes, I added HTTPPath to the options passed in at component creation time.
| @@ -80,61 +80,61 @@ func init() { | |||
|
|
|||
| // Config controls the node_exporter integration. | |||
| type Config struct { | |||
There was a problem hiding this comment.
This should really be broken up into multiple blocks (one subblock per configurable collector) since it's such a massive list of attributes, but that can be a follow up change.
|
Should we add metrics like the added in this PR? |
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
| Name: "prometheus.integration.node_exporter", | ||
| Args: node_integration.Config{}, | ||
| Exports: Exports{}, |
There was a problem hiding this comment.
This is our first component which must be a singleton until #1861 is resolved. We need to set Singleton: true here to mark it as one.
| @@ -0,0 +1,101 @@ | |||
| package node_exporter | |||
There was a problem hiding this comment.
Please go through all doc comments here and make sure they end in periods :)
component/prometheus/integrations/node_exporter/node_exporter.go
Outdated
Show resolved
Hide resolved
| targets := []discovery.Target{{ | ||
| model.AddressLabel: c.opts.HTTPListenAddr, | ||
| model.SchemeLabel: "http", | ||
| model.MetricsPathLabel: path.Join(c.opts.HTTPPath, "metrics"), | ||
| "name": "node_exporter", | ||
| }} | ||
| c.opts.OnStateChange(Exports{ | ||
| Targets: targets, | ||
| }) |
There was a problem hiding this comment.
It looks like this will be static through the lifetime of the component. You can just call this once at component creation time to reduce the amount of graph reevaluations.
component/prometheus/integrations/node_exporter/node_exporter.go
Outdated
Show resolved
Hide resolved
| c := &Component{ | ||
| log: o.Logger, | ||
| opts: o, | ||
| mut: sync.Mutex{}, |
There was a problem hiding this comment.
nit: this isn't necessary, the zero value of mut is already ready-for-use.
component/registry.go
Outdated
| for i := 0; ; i++ { | ||
| if i >= len(other) || i >= len(check) { | ||
| // one name is a complete prefix of another | ||
| return fmt.Errorf("%q cannot be used because it is incompatible with %q", check, other) | ||
| } | ||
| if other[i] != check[i] { | ||
| break | ||
| } |
There was a problem hiding this comment.
I'm finding this a little hard to follow. If you can't find a way to make it easier to follow, I think a comment explaining what's going on here will be the most useful, particularly about how the loop is always guaranteed to exit and why i >= len(other) || i >= len(check) means that the names are incompatible.
| handler := node.HTTPHandler() | ||
| if handler == nil { | ||
| w.WriteHeader(http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
This might eventually be expensive in the future since we're asking to create a new HTTP handler for every request. Could we add some kind of TODO here noting that we might want to cache the handlers in the future?
There was a problem hiding this comment.
Yeah, thats possible, but it does get changed on component update here. It would need some logic to know when to invalidate the cache.
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
github-actions
bot
added
the
frozen-due-to-age
Trying to reuse the maximum amount of code from the existing integration. The flow version simply adds hcl tags to the existing config type and invokes the integration directly.
Http handlers are a new frontier here. My first attempt here just adds a subtree at /component/{id}/ that gets delegated to the component itself if it implements HTTPComponent. This is functioning with this config:
you can read metrics at
http://localhost:12345/component/prometheus.integration.node_exporter.n_ex_test/metrics.This is a bit ugly, but it does work. If we wanted to go to in-memory pipes or whatever, the components shouldn't need to change anything, since they just expose a handler.
Fixes #1684