-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Input plugin 2673 traefik #3798
Input plugin 2673 traefik #3798
Conversation
* traefik/traefik: Update all.go clean icinga2 files add missing comma fix icinga2 test change icinga2 readme title update title Update README.md add traefik to README inputs section traefik readme traefik input plugin traefik input plugin fix test update readme icinga2 plugin unit test filter icinga2 objects to query add icinga2 to readme implement telegraf interface functions init icinga2 input plugin
plugins/inputs/traefik/traefik.go
Outdated
json.NewDecoder(resp.Body).Decode(&healthData) | ||
t.lastRequestTiming = time.Since(start).Seconds() | ||
|
||
tags := map[string]string{"server": t.Address} |
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.
since there is no option for credentials in the plugin config, i guess they need to be entered in the address field like "http://user:pass@host:port" (if so: missing config comment).
does this mean the credentials are exposed in the "server" tag ?
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.
I think we'll get this merged and it can be extended later to support basic authentication.
plugins/inputs/traefik/traefik.go
Outdated
type HttpCodes map[string]int | ||
|
||
var sampleConfig = ` | ||
# Required Traefik server address, host and port (default: "127.0.0.1") |
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.
how to add credentials?
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.
Great question. I took over this pull request to help drive it through completion. Adding credentials was not part of the original PR and not really needed to get through the initial completion of this PR. Obviously there are people out there who may want to secure this endpoint, so that functionality should probably be added. However, since the most common use case for this kind of metric data is probably unauthenticated, it makes sense to push this through as is and submit another PR with the added functionality.
Would you want to work on adding the ability to add credentials?
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.
Traefik supports several methods for authentication: https://docs.traefik.io/configuration/entrypoints/. If we add support for this (and I agree it does not need to be in the initial PR) we should do it in a way that we could configure any of the methods even if we only do HTTP Basic Auth initially.
So, if you do work on this don't use the userinfo section of the URL, instead add options like https://github.com/influxdata/telegraf/blob/master/plugins/inputs/http/http.go#L58-L60
@mkboudreau One thing I would like to add initially though is support for TLS. Should be pretty easy we just need to mirror the code in the http input: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/apache/apache.go#L100-L115
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.
@mkboudreau wrote
Would you want to work on adding the ability to add credentials?
sorry, i dont know GO, so i think i would not be helpfull.
i tried to read the mongodb input plugin code to see how they strip credentials from the server... was not able to understand it at all ;-)
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.
@jkowalleck Are you interested in HTTP Basic Auth then? I could add this in once it's merged pretty easily.
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.
@danielnelson Since traefik supports BASIC and DIGEST i suggest having both available.
see https://docs.traefik.io/configuration/entrypoints/
my current setup uses BASIC auth.
currently using httpjson
input plugin as following:
[[inputs.httpjson]] ## traefik
# ## NOTE This plugin only reads numerical measurements, strings and booleans
# ## will be ignored.
# ## Name for the service being polled. Will be appended to the name of the
# ## measurement e.g. httpjson_webserver_stats
# ##
# ## Deprecated (1.3.0): Use name_override, name_suffix, name_prefix instead.
# name = "webserver_stats"
name_override = "traefik" # overwrite target - default target is "httpjson"
# ## URL of each server in the service's cluster
# ## !!!! DO NOT ADD auth here - it might(will) be written to the (influx)
# ## output plugin as a tag
servers = ["http://traefik:57475/health"]
# ## Set response_timeout (default 5 seconds)
# response_timeout = "5s"
# ## HTTP method to use: GET or POST (case-sensitive)
# method = "GET"
# ## HTTP Headers (all values must be strings)
[inputs.httpjson.headers]
authorization = "Basic c3RhdHM6c3RhdHM="
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.
@jkowalleck I'll add support for Basic Auth after we merge, might take a bit longer to get digest. It will probably look more like the http
input (successor to httpjson in 1.6).
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.
@mkboudreau One thing I would like to add initially though is support for TLS. Should be pretty easy we just need to mirror the code in the http input: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/apache/apache.go#L100-L115
@danielnelson I should be able to update and add TLS support. Might have to wait until at least next week.
Any update on this PR? |
I was planning on doing it weeks ago, but just have not had a moment to spare. Still hoping to add in the requested changes very soon unless someone beats me to it. |
* master: (329 commits) Fix panic with unicode counter names in win_perf_counters (influxdata#4255) Update go-syslog version Update changelog Update tengine docs Restore tengine input plugin (influxdata#4160) Fix TLS and SSL config option parsing (influxdata#4247) Update changelog Use same flags for all bsd family ping varients (influxdata#4241) Ignore more boring filesystems from disk plugin (influxdata#4244) Update changelog Add SSL/TLS support to Redis input (influxdata#4236) Don't skip metrics during startup in aggregate phase (influxdata#4230) Set 1.6.4 release date Update master version to 1.8 Update sample config Add go-syslog to dependencies licenses list Update changelog Revert "Update aerospike-client-go version to latest release (influxdata#4128)" Update changelog Fix misnamed option in varnish sample config ...
@mkboudreau do you think you'll have time soon to address the latest feedback? |
@glinton sorry greg, for whatever reason I didn't realize there was anything being waited on. I'll take care of those changes right away. |
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.
I just remembered we're trying to standardize that tag. Thanks
plugins/inputs/traefik/traefik.go
Outdated
json.NewDecoder(resp.Body).Decode(&healthData) | ||
t.lastRequestTiming = time.Since(start).Seconds() | ||
|
||
tags := map[string]string{"server": t.Address} |
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.
Lets call the tag source
(rather than server
(4413)) and only use the parsed url's .Hostname()
as the value. Also, save this as a variable on the struct so it doesn't have to be computed each call to Gather
.
Are stats split by frontend/backend something that can be added or does Traefik not expose those at all? |
@danielnelson I haven't checked up on this in a while. I assumed it'd already be merged. Any idea what is preventing this PR from being merged? |
json.NewDecoder(resp.Body).Decode(&healthData) | ||
t.lastRequestTiming = time.Since(start).Seconds() | ||
|
||
tags := map[string]string{"source": t.Address} |
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.
The source
tag should only contain the hostname.
|
||
healthData := &HealthCheck{} | ||
json.NewDecoder(resp.Body).Decode(&healthData) | ||
t.lastRequestTiming = time.Since(start).Seconds() |
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.
Normally we add timing information like this as a selfstat (enabled by turning on the internal
plugin). You can see some examples of these in the influxdb_listener plugin. It may not be worth adding though since I imagine it will be quite close to the built in gather_time_ns value.
- `traefik` measurement have the following fields: | ||
- **total_count** (int) - total number of responses since the application was last restarted | ||
- **average_response_time_sec** (float64) - average time (in seconds) of all responses | ||
- **total_response_time_sec** (float64) - total time (in seconds) of all responses |
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.
For the duration types, we are trying to standardize on reporting as an integer in nanoseconds with the suffix _ns
.
- **total_count** (int) - total number of responses since the application was last restarted | ||
- **average_response_time_sec** (float64) - average time (in seconds) of all responses | ||
- **total_response_time_sec** (float64) - total time (in seconds) of all responses | ||
- **unixtime** (int) - unix timestamp of when this service was last restarted |
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.
Is this description accurate? Traefik's docs say // current server date in seconds
.
At any rate, for times we are standardizing on saving them as integers in nanoseconds since the epoch. I don't think we need to call it unixtime, something descriptive of the correct definition should work: server_time
or ``startup_time`
- **average_response_time_sec** (float64) - average time (in seconds) of all responses | ||
- **total_response_time_sec** (float64) - total time (in seconds) of all responses | ||
- **unixtime** (int) - unix timestamp of when this service was last restarted | ||
- **uptime_sec** (int) - elapsed time (in seconds) since this service was restarted |
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.
uptime_ns
# address = "http://127.0.0.1:8080" | ||
# | ||
# [inputs.traefik.tags] | ||
# instance = "prod" |
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.
Remove the tags
usage example, we don't usually show it even though it can, of course, be set on all plugins. Just showing the default config will be enough.
- **status_code_500** (int) | ||
- *status_code_*... | ||
|
||
- `traefik_status_codes` measurement have the following fields: |
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.
The cardinality of this measurement isn't going to be much of a problem, in almost all cases we are talking several dozen codes. Even 1000 series wouldn't really be a problem, so I wouldn't make it configurable, keep it simple and always send the status code metrics.
So with that in mind, and given the shared data between the two measurements, I think a single measurement would work okay if we did it similar to:
traefik,source=localhost total_count=42i,average_response_time_ns=42i
traefik,source=localhost,status_code=400 status_code_count=42i
traefik,source=localhost,status_code=500 status_code_count=42i
If we do it like this, I don't mind having them all under the same measurement because there is no overlap between the field names. However if you prefer, two measurements are still okay but we should keep the shared fields on the primary measurement only:
traefik,source=localhost total_count=42i,average_response_time_ns=42i
traefik_status_code,source=localhost,status_code=400 count=42i
traefik_status_code,source=localhost,status_code=500 count=42i
func init() { | ||
inputs.Add("traefik", func() telegraf.Input { | ||
return &Traefik{ | ||
Address: "127.0.0.1:8080", |
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.
I believe this address needs to be a URL: http://127.0.0.1:8080
I took a look at what it would take to get this up to date and merged in to the project. Digging into this I discovered there's a better option. Traefik now supports exporting data to InfluxDB directly, with many more stats, and we can even pipe that through Telegraf if we want, using the influxdb_listener plugin. For Example: Telegraf config:
Traefik config:
Going to close this now as there's nothing else to do. |
Required for all PRs:
This submission is a continuance of #2673