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.

Sign up for GitHub

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

Jump to bottom

Create TCS Handler in ecs-agent model #3731

Merged
merged 4 commits into from
Jun 20, 2023
Merged

Create TCS Handler in ecs-agent model #3731

merged 4 commits into from
Jun 20, 2023

Conversation

Realmonia
Copy link
Contributor

@Realmonia Realmonia commented Jun 2, 2023 •
edited
Loading

Summary

Create TCS Handler in ecs-agent module and corresponding agent component to integrate with this change. Also add some tests to TCS Client and TCS Handler in ecs-agent module.

Implementation details

  • ecs-agent/tcs/handler
    • Change the implementation to have a TelemetrySession interface and an implementation
    • Add a test to verify channel message persists while TCS connection dced, and resume to flow when TCS connection is recovered
  • ecs-agent/tcs/client
    • Switch from default seelog interface to structured log
    • Add a test to verify generating publish metrics from telemetry message can be paginated properly based on metric size (this case should not happen based on current metrics size)
    • Add a test to verify the telemetry message channels are non blocking
    • Add a test to verify if an invalid telemetry message is sent to channel, no problematic request will be sent (this theoretically should not happen)
    • Some naming changes
  • stats/reporter
    • Add function to generates agentVersion information (for generating TACS endpoint url), this was originally in agent/tcs/handler
    • Add function to get TCS endpoint using discoverTelemetryEndpoint API. This was originally in agent/tcs/handler
    • Embed ecs-agent/tcs/handler's TelemetrySession interface to "override" Start function, so that discoverPollEndpoint can also be part of the retry and backoff, while avoiding moving too many code (some of those are agent module specific) to ecs-agent module

A follow up PR will remove the agent/tcs/handler as well as wire in the new TCS Handler (use session definition in reporter in agent.go)

Testing

New tests cover the changes: yes

  • Testing workflow in github passed.
  • Manually verified the normal TCS workflow and TCS connect/disconnect/reconnect behavior is working fine from ecs-agent.log after using agent wired in the handler changes

Description for the changelog

Create TCS Handler in ecs-agent model

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Realmonia Realmonia changed the title [WIP] Move TCS Client to /ecs-agent [WIP] Move TCS Handler to /ecs-agent Jun 2, 2023
@Realmonia Realmonia force-pushed the tcsHandler branch 4 times, most recently from 17d496a to f5afe41 Compare June 2, 2023 23:48
@Realmonia Realmonia force-pushed the tcsHandler branch 10 times, most recently from ba77fbf to fce302e Compare June 10, 2023 01:25
@Realmonia Realmonia force-pushed the tcsHandler branch 3 times, most recently from 1440596 to 8809c11 Compare June 13, 2023 10:17
@Realmonia Realmonia changed the title [WIP] Move TCS Handler to /ecs-agent Create TCS Handler to /ecs-agent Jun 13, 2023
@Realmonia Realmonia changed the title Create TCS Handler to /ecs-agent Create TCS Handler in /ecs-agent Jun 13, 2023
Realmonia
Realmonia commented Jun 13, 2023
View reviewed changes
agent/tcs/handler/types.go
@@ -37,7 +37,6 @@ type TelemetrySessionParams struct {
CredentialProvider *credentials.Credentials
Cfg *config.Config
DeregisterInstanceEventStream *eventstream.EventStream
AcceptInvalidCert bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed because this parameter is not used. Cfg.AcceptInsecureCert is used instead.

@Realmonia Realmonia changed the title Create TCS Handler in /ecs-agent Create TCS Handler in ecs-agent model Jun 13, 2023
@Realmonia Realmonia marked this pull request as ready for review June 13, 2023 23:38
@Realmonia Realmonia requested a review from a team as a code owner June 13, 2023 23:38
Yiyuanzzz
Yiyuanzzz previously approved these changes Jun 16, 2023
View reviewed changes
RichaGangwar
RichaGangwar reviewed Jun 16, 2023
View reviewed changes
ecs-agent/tcs/handler/handler.go Outdated Show resolved Hide resolved
ecs-agent/tcs/handler/handler.go Show resolved Hide resolved
ecs-agent/tcs/handler/handler.go Show resolved Hide resolved
agent/stats/reporter/reporter.go Show resolved Hide resolved
Yiyuanzzz
Yiyuanzzz previously approved these changes Jun 19, 2023
View reviewed changes
RichaGangwar
RichaGangwar approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@RichaGangwar RichaGangwar left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Realmonia Realmonia merged commit 33570e0 into aws:dev Jun 20, 2023
@amogh09 amogh09 mentioned this pull request Jun 23, 2023
Merged
@Realmonia Realmonia deleted the tcsHandler branch June 27, 2023 22:02
@Realmonia Realmonia mentioned this pull request Jun 28, 2023
Merged
RichaGangwar
RichaGangwar reviewed Jul 7, 2023
View reviewed changes
agent/app/agent.go
return
}
go statsEngine.StartMetricsPublish()

telemetrySessionParams := tcshandler.TelemetrySessionParams{
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed it before, but why are we still using the old handler instead of the new shared one in ecs-agent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! Looks like there is a PR open yet to be merged. Will wait for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RichaGangwar RichaGangwar RichaGangwar approved these changes

@Yiyuanzzz Yiyuanzzz Yiyuanzzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Realmonia @RichaGangwar @Yiyuanzzz @amazon-ecs-bot