Update CODEOWNERS with new APM people

[watson]

No description provided.

[trentm]

Oof, I suppose. Note that I am far from grokking config loading in Kibana. Certainly I'm a good contact for Node.js APM-specific issues, however.

[trentm]

There is also https://github.com/elastic/kibana/tree/master/packages/kbn-apm-config-loader/ into which some (much?) of src/apm.js was moved a while back.

Would using this team work better? https://github.com/orgs/elastic/teams/apm-agent-node-js

[watson]

Ah yes, I didn't know @elastic/apm-agent-node-js existed. I'll update the PR to use the team.

Regarding kbn-apm-config-loader: Yeah, some of the config logic was moved to this package since I originally created the src/apm.js file. This was as far as I know mainly to facilitate that the APM agent config could be part of the regular Kibana config file instead of having its own config file.

In any case, no one are expecting you to be knowledgeable on how the Node.js agent is integrated with Kibana. But I personally felt it was nice to keep taps on changes to these files while being on the APM team as they are so intertwined.

[watson]

@elastic/kibana-core is the kbn-apm-config-loader package owned by you? I noticed that there's no codeowner for it.

If so, should you also own this file? (src/apm.js)

[trentm]

I can understand keeping tabs on changes to these files in Kibana, because it is a very helpful dogfood candidate for the Node.js APM Agent. However, does having us automatically show up as a reviewer in PRs like #83977 give the impression that those PR require an approval from us to proceed? The GH repo settings suggest that code owners do need to approve.

[watson]

The GH repo settings suggest that code owners do need to approve.

@trentm In general yes. However, you'll see that people added as reviewers because of the CODEOWNERS file usually only review the code related to the files they own. And then they leave a review like elastic/apm-agent-node-js changes LGTM 👍

I felt an ownership over this file originally because it's easy to configure the agent in a bad way so that it either doesn't work correctly or potentially slows down the application too much. So I wanted to approve changes to this file so that people didn't introduce changes that would cause these things to happen.

I know you can't of course take this on currently. So in these cases it's fine to just leave a review comment saying something like that you couldn't review because X. Then it's up to the PR owner to consider if they want to merge regardless.

[trentm]

Okay, understood. I'm fine with this then. I'm curious about an answer from kibana-core on the kbn-apm-config-loader package.

[mshustov]

Okay, understood. I'm fine with this then. I'm curious about an answer from kibana-core on the kbn-apm-config-loader package.

It should be on us as the core integrates APM in Kibana

[watson]

@restrry do you want to own the src/apm.js file as well then? If so, then I'll update this PR and add @elastic/kibana-core as codeowner of both kbn-apm-config-loader and src/apm.js.

@vigneshshanmugam In that case, do you want me to remove you as codeowner also?

[mshustov]

If so, then I'll update this PR and add @elastic/kibana-core as codeowner of both kbn-apm-config-loader and src/apm.js.

sounds good. I think it makes sense to keep @vigneshshanmugam at least in CC

[astorm]

It sounds like this was sorted out in the issue, but with the "APM Monitoring Kibana" project moving forward it feels like the Kibana (core?) team should be the ones to own this file and decide how to use the agent's feature to instrument Kibana.

[vigneshshanmugam]

I am happy to be in the loop for any change in apm configs and review those if needed. src/apm.js is also used by the RUM agent so, i am fine with keeping it as it is.

[watson]

@vigneshshanmugam @elastic/kibana-core I added you on /packages/kbn-apm-config-loader/ as well.

@astorm @trentm I removed you both.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 84120 or prevent reminders by adding the backport:skip label.