-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[elastic-agent] Re-introduce instrumentation #30385
[elastic-agent] Re-introduce instrumentation #30385
Conversation
This pull request does not have a backport label. Could you fix it @stuartnelson3? 🙏
NOTE: |
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
seems changes to metricbeat.yml were somehow picked up.
/test |
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.
It isn't critical, but I believe the code would profit from the simplifications I suggested.
x-pack/elastic-agent/pkg/agent/application/pipeline/dispatcher/dispatcher.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/controller.go
Outdated
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/emitter/emitter.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Anderson Queiroz <[email protected]>
@AndersonQ thanks for the suggestions! |
golint doesn't allow us to discard a sub-context's cancel function. Discussion is available here: golang/go#51160
@AndersonQ i updated the code as per your suggestions, thank you! the one change I made was wrt discarding the |
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.
This looks ok on my side, I haven't tested it yet.
Just want to make sure this goes in the next minor correct?
@ph Yes, definitely. And if at all possible, getting it merged before elastic-agent is migrated to its own repo and this PR needs to be re-opened there :) |
@AndersonQ Can you do a manual test of this PR so we can unblock @stuartnelson3 ? |
x-pack/elastic-agent/pkg/agent/application/pipeline/dispatcher/dispatcher.go
Show resolved
Hide resolved
x-pack/elastic-agent/pkg/agent/application/pipeline/dispatcher/dispatcher.go
Outdated
Show resolved
Hide resolved
/test |
@ph @AndersonQ any idea what the failures are? they only started with 61da5fc but i have no idea why. from the e2e failure:
|
@stuartnelson3 I've never seen this on, not sure what |
/test |
It does ring a bell, but I don't quite remember. Anyway I haven't found this log on the current build logs.
what seems to be correct, when the elastic-agent uninstalls itself, it removes the perhaps ping the robots, they're the ones owning the e2e tests. |
I'd say the error could be related to a network glitch while downloading the elastic-agent from Elastic's artifacts API:
It happens at the very beginning of the execution of that stage, which correlates with the failing scenario (the first one that is executed). After that, further downloads of the binary works. I'd say that the error is not caused by this PR and could be ignored, as any other execution should work. |
@ph @AndersonQ how do you two feel about @mdelapenya 's comment? this sounds like a transient network error, so I'll re-run to see if it works |
/test |
@ph and @AndersonQ please take some time to support getting this merged in before the agent is moved to the new repo. This has been ongoing for quite a while and I think the important part is to get the base merged. Any further details could be ironed out on a follow up; I assume you would like to extend the instrumentation over time anyways. |
The current e2e test failure is under our radar: elastic/e2e-testing#2203 |
This pull request is now in conflicts. Could you fix it? 🙏
|
@stuartnelson3 @simitt Sorry for the delay, I was looking at an ongoing SDH, that took way more time than I wanted to solve. Looking a the last changes it look good to me @AndersonQ can you do a final review? |
Thanks for the feedback @ph. @stuartnelson3 is on PTO, can you or @AndersonQ please take ownership of merging this in or specifying what exactly is missing? |
I see that the Elastic Agent has moved to it's own repository. @ph can you please own transfering this PR over and get it over the finish line? |
my PTO is being shifted slightly so I'll take a look at transferring this PR over today. I'll catch up with @ph if there is anything I need help with when transferring |
This pull request is now in conflicts. Could you fix it? 🙏
|
Going to close this one in favor of the one on the Elastic Agent repository. |
cf. #29031 for original description.
seems the issue might have been forgetting to add credentials when starting up the gRPC servers: 7638367