Skip to content
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

Feature/sideload http #1894

Merged
merged 11 commits into from
Dec 17, 2020
Merged

Conversation

jregovic
Copy link

@jregovic jregovic commented Apr 19, 2018

The HTTP scheme is added as a valid option for the source in a sideload node.

The HTTP source should return a JSON object where each property is a key used in the sideload cache and has a set of properties to be used by sideload:
{
"server1": {
"cpu_threshold": 99.9,
"disable": "false"
},
"server2": {
"cpu_threshold": 85,
"disable": "true"
},
"server3": {
"cpu_threshold": 56,
"disable": "false"
}
}

Tested using the following TICK:

stream
|from()
.database('telegraf')
.retentionPolicy('autogen')
.measurement('cpu')
.groupBy(*)
|sideload()
.source('http://localhost:5000/threshold/')
.order('{{ .host }}')
.field('cpu_threshold', 0.0)
.tag('disable','false')
.tag('custom', 'false')
|where(lambda: !bool("disable"))
|alert()
.details('')
.message('Sideload alert triggered on threshold {{ index .Fields "cpu_threshold" | printf "%0.2f" }} with value {{ index .Fields "usage_idle" | printf "%0.2f" }}')
.crit(lambda: "usage_idle" > float("cpu_threshold"))
.log('/alerts.log')

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is headed in the right direction.
A couple of thoughts:

  • How does this source work with more than one entry in the order list? It will need to support the hierarchy concept like the file store does.
  • This is designed such that only a single request is made during startup and when a reload signal has been received. This is good, but we should make it clear in the docs that it is a cache and does not make a GET request for each data point.

Thanks!

@@ -35,6 +35,9 @@ type SideloadNode struct {
// Tags is a list of tags to load.
// tick:ignore
Tags map[string]string `tick:"Tag" json:"tags"`

HttpUser string `json:"httpuser"`
HttpPassword string `json:"httppassword"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer these not be part of the TICKscript so that we do not encourage users to place credentials in their scripts.

There is already a concept of httppost endpoints that allow the configuration of URLs and credentials, should we leverage that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had not considered the use of endpoints. That would be abetter way to handle authentication.

I would assume that the ordering would work since I tried to implement it as an absrraction of the existing code, but I will definitely test it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to create a new configuration section for the sideload, try and re-use what is available for httppost, or create a more generic endpoint configuration?

The httppost configuration could be used unmodified, I think, but then the dual uses of httppost in the configuration would be obfuscated and confusing.

@jregovic
Copy link
Author

I am thinking that it might be useful to add an option for an endpoint, like httpPost, and still allow for a URL to be included as the source, with the URL being assumed to be an unauthenticated GET, and the endpoint being used for authenticated requests.

So there could be:
|sideload() .source() .endpoint('example')

or
|sideload() .source('file://PATH')

or
|sideload() .source('http://PATH')

@jregovic
Copy link
Author

I am attempting to re-use the Endpoint definitions from the config file, but I am getting a bunch of errors because I can't reference the unexported field.

I figure I could have the sideload node take the value for the source and see if it is an endpoint from the config by using
et.tm.HTTPPostService.Endpoint(n.Source)

But then I am unable to reference the fields in the returned data structure. I am obviously missing sometinhg here. Any ideas?

@jregovic
Copy link
Author

I've added updates to use [[httppost]] definitions as a source when the source is specified as a non-uri. Updated documentation as well.

Using httpost endpoints required modifying the httppost definitions to export the Url and Auth fields. Added support for HTTPS as well, but there is no option for skipping certificate verification.

Tested the following configurations:

  • Source in JSON files
  • Source in YAML files
  • Source as URL
  • Source as [[httppost]] endpoint

Support HTTPS

Update tests for new format.
@jregovic jregovic force-pushed the feature/sideload-http branch from 4bae602 to 614254d Compare June 1, 2018 16:34
@jregovic
Copy link
Author

jregovic commented Jun 1, 2018

@nathanielc Does this make more sense?

@dp1140a
Copy link

dp1140a commented Jun 22, 2018

Bump

@jregovic
Copy link
Author

bump

@jregovic
Copy link
Author

jregovic commented Aug 2, 2018

Hello?

@jregovic jregovic force-pushed the feature/sideload-http branch from 407f2aa to 6d2e47b Compare March 21, 2019 13:02
@jregovic
Copy link
Author

Any input on whether this would be accepted as a new feature?

@docmerlin
Copy link
Contributor

I am good with everything before the merge commit, but the merge commit here is pretty substantial, so someone else should also take a look.

Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work switching to inheritance to share more code and allow other source types to be defined.

@docmerlin docmerlin merged commit f1ff9ab into influxdata:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants