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

[gh-action]: Generate a weekly report of activity #26111

Merged
merged 37 commits into from
Sep 29, 2023

Conversation

kevinslin
Copy link
Contributor

Description:

This PR creates a gh action that generates a weekly report on repo statistics.
It delivers on the requirements specified in #24672
You can see the sample output here: kevinslin#16

Link to tracking Issue: #24672

Testing:

Manual testing in fork: https://github.com/kevinslin/opentelemetry-collector-contrib/actions
Example output: kevinslin#17

Documentation:

The architecture:

  • we use actions/github-script@v6 to make calls to the gh apis
  • we require installing js-yaml in order to parse metadata.yaml files in order to get components. this dependency is installed during the github action run and not persisted

Some caveats about the logic:

  • when this action runs, it looks back the previous 7 days and gets issues created in that time period, normalizing times to UTC
    • eg. if running this on wednesday (eg. 2023-08-25 17:35:00), it will scan issues from the previous wednesday (2023-08-18 00:00:00Z) to beginning of this wednesday (2023-08-28 0:00:00Z)
  • this action writes the json payload of the report inside the issue. the payload is parsed by future reports to calculate deltas
  • the report issue has a custom label: report - this is used so we can properly filter previous issues when calculating deltas. the github issues api only does filtering based on labels and since date

This action currently runs every Tuesday at 1AM UTC

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@kevinslin
Copy link
Contributor Author

didn't add a changelog entry since this was an internal facing change. happy to add an entry if we feel this warrants that

@atoulme atoulme added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 26, 2023
@kevinslin
Copy link
Contributor Author

updated issue format with gh style checkboxes: kevinslin#21

@kevinslin
Copy link
Contributor Author

looks like the gh workflows requires an approval from a maintainer?

// Check if directory exists
let files;
try {
files = fs.readdirSync(startPath);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the async versions of the fs functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the logic was easier to follow when sticking with sync. since the number of directories isn't that large, felt like this wouldn't be a bottleneck

Copy link
Member

Choose a reason for hiding this comment

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

the logic was easier to follow when sticking with sync.

Not sure I understand. The flow of the logic is the same with async, you just add the await keyword.

    files = await fs.readdir(startPath);

The await keyword is already used elsewhere in the script, e.g.

const response = await octokit.issues.listForRepo(queryParams);

I thought maybe it's an issue in importing the fs/promises version of the fs library?

const results = {};

for (let filePath of files) {
const component = path.basename(path.dirname(path.dirname(filePath))); // Top level directory
Copy link
Member

@andrzej-stencel andrzej-stencel Sep 11, 2023

Choose a reason for hiding this comment

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

If I understand correctly, by component you mean component class - a receiver, or a processor, etc. Reading the component type from the path should work, but a cleaner way would be to use the status.class field from the metadata file contents.

You could also read the component's name from type field in the metadata file.

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 is because we currently collect metadata.yaml from everything. this includes internal components that don't have a status.class field (eg. processor/resourcedetectionprocessor/internal/aws/ec2/metadata.yaml). though to be fair. added a check for the status.class to set component and default to the current method if nothing is found

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

All my comments are minor things that can be either ignored or improved upon in separate PRs. This is good to be merged in my view 👍

@kevinslin kevinslin force-pushed the chore/weekly-report-gen branch from 0f693a0 to 7a11c9e Compare September 12, 2023 22:22
@kevinslin
Copy link
Contributor Author

finished addressing pr changes. thanks for the review @astencel-sumo

@kevinslin
Copy link
Contributor Author

checking if there are any other blockers for this pr? @atoulme do you have authorization to merge this pr?

.github/workflows/generate-weekly-report.yml Outdated Show resolved Hide resolved
.github/workflows/scripts/generate-weekly-report.js Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

I am a little concerned that this introduces javascript to our repository. @open-telemetry/collector-contrib-approvers is everyone willing to take on this language dependency?

@kevinslin
Copy link
Contributor Author

I am a little concerned that this introduces javascript to our repository. @open-telemetry/collector-contrib-approvers is everyone willing to take on this language dependency?

its a valid concern. i just want to point out that there is already javascript in the repository (embedded inside of yaml).
see here and here

@andrzej-stencel
Copy link
Member

andrzej-stencel commented Sep 20, 2023

I am a little concerned that this introduces javascript to our repository. @open-telemetry/collector-contrib-approvers is everyone willing to take on this language dependency?

I've worked with some decently large Node.js apps in my past, I'd be willing to take it.

Unless we'd rather have this report generator written in a different language, that more contributors of this repo are comfortable with. What would it be? Perhaps Python?

@atoulme
Copy link
Contributor

atoulme commented Sep 22, 2023

I'm fine helping with js code.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Sep 22, 2023
@TylerHelmuth
Copy link
Member

Gonna give one more day for approvers/maintainers to speak up, then I'll merge.

@evan-bradley
Copy link
Contributor

Speaking as someone who has written a handful of GitHub workflows as Bash scripts that call the GitHub CLI, I think writing workflows in a programming language that use an API client library is a better alternative: it's much easier to reason about and should be more straightforward to test locally. I have pretty extensive JavaScript experience, so I would be happy to help maintain JavaScript workflows.

Looking at our options, between the three language implementations that GitHub provides SDKs for, I think JavaScript is the most likely to be the best understood both within the Collector SIG and across the wider OTel community (I'd like to maintain an eye toward possibly publishing our actions to be used in other repos). I think using Go would also be an obvious choice, and there is a third-party API client for GitHub written in Go, but I can't comment on its quality compared to the official JavaScript SDK.

@TylerHelmuth TylerHelmuth merged commit e31238f into open-telemetry:main Sep 29, 2023
96 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 29, 2023
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
**Description:** 

This PR creates a gh action that generates a weekly report on repo
statistics.
It delivers on the requirements specified in
open-telemetry#24672
You can see the sample output here:
kevinslin#16

**Link to tracking Issue:** open-telemetry#24672

**Testing:** 

Manual testing in fork:
https://github.com/kevinslin/opentelemetry-collector-contrib/actions
Example output:
kevinslin#17

**Documentation:** 


The architecture:
- we use `actions/github-script@v6` to make calls to the gh apis
- we require installing `js-yaml` in order to parse `metadata.yaml`
files in order to get components. this dependency is installed during
the github action run and not persisted

Some caveats about the logic:
- when this action runs, it looks back the previous 7 days and gets
issues created in that time period, normalizing times to UTC
- eg. if running this on wednesday (eg. 2023-08-25 17:35:00), it will
scan issues from the previous wednesday (2023-08-18 00:00:00Z) to
beginning of this wednesday (2023-08-28 0:00:00Z)
- this action writes the json payload of the report inside the issue.
the payload is parsed by future reports to calculate deltas
- the report issue has a custom label: `report` - this is used so we can
properly filter previous issues when calculating deltas. the [github
issues
api](https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues)
only does filtering based on labels and `since` date

This action currently runs every Tuesday at 1AM UTC

---------

Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants