elastic / package-spec

EPR package specifications
Other
17 stars 71 forks source link

[Change Proposal] Introduce data stream level 'owner' field #658

Closed tommyers-elastic closed 1 month ago

tommyers-elastic commented 10 months ago

There are several packages where data streams are owned by different teams. For example, the azure package has data streams owned by obs-integrations, obs-platforms, and security-external-integrations teams.

Currently we have a spreadsheet detailing package ownership, but it is hard to find, and not kept up to date. We also have team ownership detailed in the CODEOWNERS file in the integrations repo, but it is also not kept up to date, and does not provide good visibility into ownership for anyone browsing the repo.

A data stream level owner field is proposed to improve visibility and become the source of truth for data stream level ownership. An additional CI task should also be created in the integrations repo which validates that CODEOWNERS is kept up to date with this new field, ensuring any automation (issue routing, code review requests etc) uses the correct team label.

jsoriano commented 10 months ago

This proposal has been rejected in the past, more recently in https://github.com/elastic/integrations/issues/7023

I agree that there is space for improvement in this area, because this is a recurrent discussion, I only wonder if introducing more ownership declarations is going to help.

To me the main point is that I think that it is better to have a single point of contact per package, this single point of contact can forward requests to the most appropriate team or person. And for internal organizational separation, CODEOWNERS has worked so far. Another point is that ownership separation can be functional, for example we have packages whose dashboards are maintained by a different team. Then where would we stop in the owners granularity?

Currently we have a spreadsheet detailing package ownership, but it is hard to find, and not kept up to date.

I didn't know about this spreadsheet. Does it contain information that is not in the repo? Would it help to automate the creation of some kind of table from the information in manifests and CODEOWNERS and show it in the integrations repo?

We also have team ownership detailed in the CODEOWNERS file in the integrations repo, but it is also not kept up to date

Why is it not updated? What guarantees that we would keep the owner in data streams updated?

tommyers-elastic commented 10 months ago

interesting point about per-dashboard ownership - i wasn't aware of this. do we have examples of this?

I didn't know about this spreadsheet

Sorry, I should have clarified that this is something we have internally for obs owned packages. but you prove my point, it's not visible at all :)

I think my main concern here is that we decide on a single source of truth. Maybe we can choose CODEOWNERS, but to me having this in the spec feels more concrete, and we can then generate CODEOWNERS from the package definitions (or at least check that it's up to date in PRs).

Why is it not updated? What guarantees that we would keep the owner in data streams updated?

Do we need to add a check box to the PR template or something?

jsoriano commented 10 months ago

interesting point about per-dashboard ownership - i wasn't aware of this. do we have examples of this?

The dashboards of some packages are co-owned by the kibana visualizations team, for example for the system package: https://github.com/elastic/integrations/blob/c1b069938b6bf65cdec6295b1004e763b1746a6b/.github/CODEOWNERS#L228 And looking to the git history, they indeed contribute to them.

I think my main concern here is that we decide on a single source of truth. Maybe we can choose CODEOWNERS, but to me having this in the spec feels more concrete, and we can then generate CODEOWNERS from the package definitions (or at least check that it's up to date in PRs).

Well, we have now two different sources of truth, but they are meant for different things, so there wouldn't be duplication:

Well, the spreadsheet would be another source of truth, but I think we can discuss about generating something automatically from the ones above.

having this in the spec feels more concrete

We also have to think on what we would need to do to make this useful. We would need to make it visible, what might imply changes in the package registry, documentation, integrations UI and so on. And for external people this may be confusing, as they may find multiple points of contact for the same package.

Why is it not updated? What guarantees that we would keep the owner in data streams updated?

Do we need to add a check box to the PR template or something?

Yep, this could be an option. But usually changes in ownership are organizational, and not related to changes in code. I guess this would be something leads should take into account when there are changes in teams.

tommyers-elastic commented 10 months ago

Yep, this could be an option. But usually changes in ownership are organizational, and not related to changes in code. I guess this would be something leads should take into account when there are changes in teams.

i'm thining more along the lines of, if a new data stream is added, and the author of the package is not the overall package maintainer, there should be a reminder to append a line to CODEOWNERS.

agree with everything else in the above. next steps seem to be documentation related: better define usage of CODEOWNERs vs owner.github and communicate internally. request that any spreadsheets detailing internal ownership are not used as the source of truth, and that CODEOWNERS is up to date. what do you think?

lalit-satapathy commented 10 months ago

The owner in package manifests is the "main" owner of the package, it is the general owner of the package, making decisions on the development of the package and so on, and the single point of contact for support, users and other external people.

This single point of contact model, is challenging for some of the complex packages and does not enable efficient support routing. For example: aws has 40 data streams (which are structured as individual integrations per each data stream) and these data streams ownerships span across multiple o11y and security teams.

To me the main point is that I think that it is better to have a single point of contact per package, this single point of contact can forward requests to the most appropriate team or person. And for internal organizational separation, CODEOWNERS has worked so far.

We would also like to avoid support routing happening to a single team for such packages, which again re-routes to individual data streams owners. Instead the support routing should be done to the owner team directly, to avoid delay in the process.

Following approach can be used resolve this urgent issue:

jsoriano commented 10 months ago

The owner in package manifests is the "main" owner of the package, it is the general owner of the package, making decisions on the development of the package and so on, and the single point of contact for support, users and other external people.

This single point of contact model, is challenging for some of the complex packages and does not enable efficient support routing. For example: aws has 40 data streams (which are structured as individual integrations per each data stream) and these data streams ownerships span across multiple o11y and security teams.

One particularity that have these complex packages is that they are presented as multiple tiles to the user. Maybe for these cases we could add ownership "per tile". I would be more ok with that because it would be more clear where and how to show this information, and in my opinion it would make more sense from a product perspective. But I don't know if tiles ownership is also split, for example are ec2_metrics and ec2_logs owned by the same team? If tiles ownership is also split this model would not work.

short-term: Manually add data stream owner in CODEOWNER file as done here

+1, but I would add a mid-term step to re-evaluate if something else is needed after this. The automatic generation of the CODEOWNER as suggested for the long-term would still require to add support for onwership in data streams and manually add this ownership to each package.

If we decide that CODEOWNER is not enough, and go in the direction of adding ownership per data stream, or per tile, we also need to clarify how this new information is going to be consumed.

tommyers-elastic commented 10 months ago

the per tile option is interesting - this would be a new field in the policy templates section? i actually think our internal model on intra-package ownership is based on this. in our spreadsheet we just list "cloudwatch".

the question now though comes back to which is the source of truth? what if CODEOWNERS conflicts with the owner field for the tile in the manifest?

jsoriano commented 10 months ago

the per tile option is interesting - this would be a new field in the policy templates section? i actually think our internal model on intra-package ownership is based on this. in our spreadsheet we just list "cloudwatch".

Yes, as "tiles" are defined now, it would be a new field in the policy template.

the question now though comes back to which is the source of truth? what if CODEOWNERS conflicts with the owner field for the tile in the manifest?

We could check that the owner in the policy template is also owner of the directories of the listed data streams, as we do now with package owners.

tommyers-elastic commented 10 months ago

We could check that the owner in the policy template is also owner of the directories of the listed data streams, as we do now with package owners.

oh cool - where is that check performed? do you have a link to the code?

jsoriano commented 10 months ago

We could check that the owner in the policy template is also owner of the directories of the listed data streams, as we do now with package owners.

oh cool - where is that check performed? do you have a link to the code?

It is part of a magefile target that is called by CI, here: https://github.com/elastic/integrations/blob/536fd1e141121923d7b0618b48643afd879077b8/dev/codeowners/codeowners.go#L24

andrewkroh commented 2 months ago

I was considering to reopen the discussion on per-data stream ownership, but after reading the history here I'd like to propose adding some additional "lint" checks on the CODEOWNERS file to help clarify ownership. My goal is to remove ambiguity about who is responsible for a data stream.

The rules I would like to enforce affect packages with shared ownership across data streams. The rules are:

Again, these apply only to packages where the ownership is divided. This would be determined by reading the number of owners listed for a package's root directory in the CODEOWNERS file.

This would eliminate the various ownership spreadsheets that are maintained(?) out-of-band. The ownership changes would have a clear audit trail and be reviewable by the involved parties.

jsoriano commented 2 months ago

I would be fine with this kind of check, yes, we could add the new logic to the current checks we have.

tommyers-elastic commented 2 months ago

agree - this sounds like a good step

pkoutsovasilis commented 2 months ago

The rules I would like to enforce affect packages with shared ownership across data streams. The rules are:

  • Require exactly one owner per data stream directory.
  • Require that every data stream directory be list explicitly with an owner.

@andrewkroh I am not so sure how this rule Require exactly one owner per data stream directory. applies to this one. Any ideas?

pkoutsovasilis commented 1 month ago

hello 👋 since this https://github.com/elastic/integrations/pull/10310 is now merged should we close this issue?

jsoriano commented 1 month ago

Yeah, I think this can be closed, thanks @pkoutsovasilis!