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

inputs.ecs: add v3 metadata support. #7154

Merged
merged 3 commits into from
Jul 7, 2020

Conversation

sergeykhegay
Copy link
Contributor

@sergeykhegay sergeykhegay commented Mar 11, 2020

Justification: v2 metadata and Docker stats are available to tasks that use the awsvpc network mode ... only [v2]. v3 metadata is available regardless of the network mode and provides the same statistics as the v2 metadata.

[v2] https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v2.html

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@sergeykhegay
Copy link
Contributor Author

Could you take a look, @danielnelson?

@sergeykhegay
Copy link
Contributor Author

Maybe you could peek, @ssoroka?

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks reasonable. 👍

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

I think having both endpoint_url and metadata_version options introduce option combinations that you wouldn't normally want and would make the plugin more difficult to setup. For example, during normal use you would never want to set metadata_version = 3 and leave the endpoint_url as the default value. Likewise setting an empty endpoint_url = "" and metadata_version = 2 wouldn't work either.

What if we removed the metadata_version and on startup we try to read the ECS_CONTAINER_METADATA_URI envvar, if this is set we use v3 and if not we try with v2 and the endpoint_url. We should update the endpoint_url option to mention that it is only for use with v2.

With this approach most users will be bumped forward to v3, so the idea requires that v3 needs to show the same results as v2. I believe this is the case. If one wanted to force v2, it would always be possible to unset the environment variable.

Comment on lines 98 to 99
path := getMetaStatsPath(c.version)
c.statsURL = c.BaseURL.String() + path
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but I prefer the old style:

c.statsURL = c.BaseURL.ResolveReference(getMetaStatsPath(c.version)).String()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this doesn't work with v3 metadata endpoint.

The url for v3 looks something like http://1.1.1.1/v3/metadata (I do not recall exact path, but the important part is that the path is present. IP is not relevant). When you use ResolveReference on such base URL, the path you provide will override the the original path. I faced this issue when I was testing the code in ECS.

A small demo:

package main

import (
	"fmt"
	"log"
	"net/url"
)

func main() {

	examples := []struct{
		base string
		path string
	} {
		{
			base: "http://1.1.1.1/v3/metadata",
			path: "/stats",
		},
		{
			base: "http://1.1.1.1/v3/metadata",
			path: "stats",
		},
	}
	
	for i, e := range examples {
		u, err := url.Parse(e.path)
		if err != nil {
			log.Fatal(err)
		}
		base, err := url.Parse(e.base)
		if err != nil {
			log.Fatal(err)
		}
		fmt.Println("Example ", i)
		fmt.Println(" base:   ", e.base)
		fmt.Println(" path:   ", e.path)
		fmt.Println(" result: ", base.ResolveReference(u))
		fmt.Println()
	}
}

Result:

Example  0
 base:    http://1.1.1.1/v3/metadata
 path:    /stats
 result:  http://1.1.1.1/stats

Example  1
 base:    http://1.1.1.1/v3/metadata
 path:    stats
 result:  http://1.1.1.1/v3/stats

@danielnelson danielnelson added this to the 1.15.0 milestone Mar 24, 2020
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 24, 2020
@sergeykhegay
Copy link
Contributor Author

Just noticed the change request. Will fix it. This is not abandoned.

@snemetz
Copy link

snemetz commented Apr 15, 2020

There is also metadata v4 now with a new variable name.
Would be nice if this could handle any of the versions

@danielnelson
Copy link
Contributor

@snemetz Thanks for the heads up, and I sure hope these version changes settle down. It would be good to support all available versions if we can. Could you open a feature request issue for v4 metadata?

@snemetz
Copy link

snemetz commented Apr 17, 2020

v4 feature request created

@sergeykhegay
Copy link
Contributor Author

sergeykhegay commented Apr 18, 2020

@danielnelson, pushed requested changes. Please take a look.

@danielnelson danielnelson self-requested a review June 13, 2020 01:05
@danielnelson danielnelson added the area/aws AWS plugins including cloudwatch, ecs, kinesis label Jul 2, 2020
@sergeykhegay
Copy link
Contributor Author

@danielnelson did you have a chance to peek again?

@danielnelson danielnelson merged commit 55b672e into influxdata:master Jul 7, 2020
@danielnelson
Copy link
Contributor

Thank you!

rhajek pushed a commit to bonitoo-io/telegraf that referenced this pull request Jul 13, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aws AWS plugins including cloudwatch, ecs, kinesis feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants