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

📊 introduce metrics for the peribolos app #128

Merged
merged 1 commit into from
May 30, 2022

Conversation

harshad16
Copy link
Member

introduce metrics for the peribolos app
Signed-off-by: Harshad Reddy Nalla hnalla@redhat.com

Related-to: #58
#59

  • Created metrics.md inspired from https://github.com/AICoE/meteor/blob/main/docs/metrics.md
  • Used Server method from probot instead of run, for custom route like /metrics
  • Included a few metrics for peribolos for num of installation, uninstallation, current no of user serving, operation results
  • include any type for the variable it was prompting.

Tested on local:
peribolos-metrics

@sesheta sesheta added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 17, 2022
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I love this ❤️ 💯
This is great content!

I have inlined few suggestions to address. I like it a lot though! 👍

src/probotOnKube.ts Outdated Show resolved Hide resolved
src/metrics.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Probot: probotDefault,
})
: new Server({
webhookProxy: process.env.WEBHOOK_PROXY_URL,
Copy link
Member

Choose a reason for hiding this comment

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

This does not start the webhook proxy by itself. It doesn't instantiate the smee-client the same way as run() call does. I think you're missing something like:

const smee = process.env.WEBHOOK_PROXY_URL
  ? undefined
  : new SmeeClient({
      source: process.env.WEBHOOK_PROXY_URL || '',
      target: 'http://localhost:3000/events',
      logger: console,
    });

const events = smee?.start();
...
process.env.WEBHOOK_PROXY_URL &&
  [
    `exit`,
    `SIGINT`,
    `SIGUSR1`,
    `SIGUSR2`,
    `uncaughtException`,
    `SIGTERM`,
  ].forEach((_) => {
    events?.close();
  });

to work aroud this.

Copy link
Member Author

Choose a reason for hiding this comment

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

i have tested this out with my own smee setup, it works for me
is this something that didn't work for you?


run(app);
async function startServer() {
const probotDefault = Probot.defaults({
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use createProbot() func instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

src/index.ts Show resolved Hide resolved
src/metrics.ts Outdated Show resolved Hide resolved
src/metrics.ts Outdated Show resolved Hide resolved
@tumido
Copy link
Member

tumido commented May 30, 2022

/approve

Additional changes will be addressed in a follow up PR.

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2022
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label May 30, 2022
@harshad16
Copy link
Member Author

/hold
will update the pr soon

@sesheta sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2022
@sesheta sesheta removed the lgtm Indicates that a PR is ready to be merged. label May 30, 2022
Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label May 30, 2022
@sesheta
Copy link
Member

sesheta commented May 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tumido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tumido
Copy link
Member

tumido commented May 30, 2022

/unhold

@sesheta sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2022
@sesheta sesheta merged commit 84644e6 into operate-first:main May 30, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.0.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Marks issues as released label Jun 21, 2022
@tumido tumido mentioned this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. released Marks issues as released size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants