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

ecs_task_observer: initial structure #6121

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

rmfitzpatrick
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick commented Nov 3, 2021

Description:
Adding a feature - These changes introduce a new receiver creator-compatible watch observer for ECS tasks. It differs from the existing ECS observer in that it supports only sidecar deployments and doesn't have a hard dependency on the Prometheus receiver, instead being able to sync with a receiver creator instance by listing container endpoints. This approach was landed on after reviewing the existing and ECS observer config and implementation and from following the guidance of the initial observer's author: #5316 (comment). It does not resolve the outstanding item for #1395, as I think observer endpoint creation should be added to the existing observer as well but should be done in unrelated changes.

Link to tracking Issue:
#5316

Testing:
No tests as this time since the changes just include the initial types and readme. Basic factory test to help ensure receiver creator type compatibility.*

Documentation:
Adding a readme with high level plan and config documentation generated by project configschema tool.

cc @alolita

@bogdandrutu
Copy link
Member

@Aneurysm9 would be good someone from AWS to review/own(coown) this

extension/observer/ecstaskobserver/extension.go Outdated Show resolved Hide resolved
extension/observer/ecstaskobserver/factory.go Outdated Show resolved Hide resolved
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

This looks like a nice addition. @rakyll I know you've talked about ways to improve ECS task detection, does this align with the direction you'd expect?

@Aneurysm9
Copy link
Member

Using versioning file /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/versions.yaml
verifyAllModulesInSet failed: Module github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/ecstaskobserver (defined in /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/extension/observer/ecstaskobserver/go.mod) is not listed in any module set.

Please add this new module to versions.yaml.

extension/observer/ecstaskobserver/extension.go Outdated Show resolved Hide resolved
)
require.NoError(t, err)
require.NotNil(t, eto)
asObservable, ok := eto.(observer.Observable)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using https://pkg.go.dev/github.com/stretchr/testify/assert#Implements to check if it implements the interface.

However, this feels like a redundant tests considering there is a compile time check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is for the factory method which wouldn't enforce the returned helper type at compile time, though the extension itself does have an added implementation check that could make this redundant. Given the level of indirection I think it's a helpful assurance.

Updated to use the implementation helper.

@rmfitzpatrick rmfitzpatrick force-pushed the ecstaskobserverinit branch 2 times, most recently from cb3cec4 to a5ec000 Compare November 9, 2021 14:55
@rmfitzpatrick
Copy link
Contributor Author

I believe this is ready to land, and the tests failures have been unrelated.

@tigrannajaryan tigrannajaryan merged commit 305d7c0 into open-telemetry:main Nov 11, 2021
@tigrannajaryan
Copy link
Member

@rmfitzpatrick please populate the commit message in future PRs (the PR description is typically great to use the commit message). I copy/pates this time before merging.

rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Nov 29, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Nov 29, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Nov 30, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 1, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 3, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 7, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
rmfitzpatrick pushed a commit to rmfitzpatrick/opentelemetry-collector-contrib that referenced this pull request Dec 7, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
bogdandrutu pushed a commit that referenced this pull request Dec 8, 2021
These changes implement the new ECS task observer extension defined by #6121.  They don't adopt it as an available component by the service though.
PaurushGarg pushed a commit to open-o11y/opentelemetry-collector-contrib that referenced this pull request Dec 11, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
PaurushGarg pushed a commit to open-o11y/opentelemetry-collector-contrib that referenced this pull request Dec 11, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
PaurushGarg pushed a commit to open-o11y/opentelemetry-collector-contrib that referenced this pull request Dec 11, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
PaurushGarg pushed a commit to open-o11y/opentelemetry-collector-contrib that referenced this pull request Dec 14, 2021
These changes implement the new ECS task observer extension defined by open-telemetry#6121.  They don't adopt it as an available component by the service though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] [Extension] ECS Service Discovery for Prometheus
5 participants