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.

Sign up for GitHub

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

Jump to bottom

[observability] Migrate Gitpod's mixin #5843

Merged
merged 3 commits into from
Oct 5, 2021
Merged

[observability] Migrate Gitpod's mixin #5843

merged 3 commits into from
Oct 5, 2021

Conversation

ArthurSens
Copy link
Contributor

@ArthurSens ArthurSens commented Sep 23, 2021 •
edited

Description

This PR migrates Gitpod's mixin from https://github.com/gitpod-io/observability to this repository.

This is being done with the goal of improving DevX when it comes to our own observability. With this migration(plus the work being done in #5745), we can introduce/modify Prometheus metrics while also making the necessary changes in Grafana dashboards and Prometheus alerting rules all in the same PR.

PS: Recommended to add /werft with-observability to future PRs so one can see metrics changes from code instrumentation all the way to Grafana and Prometheus alerts

Other changes

The migration did require some extra changes:

  • To be able to put this mixin under the MIT License, the addlicense script was modified to support .jsonnet and .libsonnet files.
  • To be able to split the Ownership of mixins per team, the mixin itself was also split into a different folder structure. Where in the previous repository they were all mixed together. This folder structure is also reflected in Grafana itself!
  • I'm setting myself as the owner of the whole operations/observability/mixins folder. I'm doing this so I can help review every PR that modifies our mixins. We can change that once each team has confidence enough to maintain the mixin without my help.

Related Issue(s)

Fixes gitpod-io/observability#192

How to test/review

I'd start the review by reading the README.md file added in the operations/observability/mixins folder. It does contain written docs, references to official documentation and even a video with a quick tour to Grafana's UI.

After README, I'd go through the changes made to the werft job. More specifically, the parts that handle the jsonnetfile.json from our observability repo. This is where monitoring satellite is updated with changes made by a specific PR and added to the preview environment.

The next step would be going to Grafana of the preview environment and seeing the new folder structure. It is possible that some dashboards are in the wrong folder, by I'd like to keep it that way in this PR. An exercise to get familiar with the mixins would be someone from each team to open PR fixing the folder locations (Of course I'd be present during this activity to answer any questions)

I'm also happy to jump in a call with anyone to help clarify any doubts that may appear in the review process.

Release Notes

NONE

/werft with-observability
/werft withObservabilityBranch=arthursens/separate-dashboards-272

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@ArthurSens
Copy link
Contributor Author

/hold

There is no way to test this with previews until #5745 gets merged

@ArthurSens ArthurSens changed the title As/migrate mixins [observability] Migrate Gitpod's mixin Sep 23, 2021
@meysholdt
Copy link
Member

Thoughts from a discussion with @csweichel :

About this PR:

  • can we use the path operations/observability instead of dev/mixins?
  • I'll clarify under which license we want to publish this.

General:

  • We should look if we can relieve colleagues from having to learn Jsonnet by allowing them to define alerts as k8s CRs and Dashboards as pure JSON.
  • It would be nice to have howtos about How to add metric, how add a graph, how to add an alert.
  • it's not so nice to have code in this repo that depends on private code. let's discuss how we can improve that.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@ArthurSens
Copy link
Contributor Author

ArthurSens commented Sep 24, 2021 •
edited by werft-gitpod-dev-com bot

/werft run

👍 started the job as gitpod-build-as-migrate-mixins.7

@csweichel
Copy link
Contributor

csweichel commented Sep 24, 2021 •
edited

image

known issue?

@ArthurSens
Copy link
Contributor Author

ArthurSens commented Sep 24, 2021 •
edited

image

known issue?

Nops, I'll look into it 😬

EDIT: I just replaced go install with go get inside the Makefile and it worked, but go get is being deprecated soon.

It is a known issue that prometheus own go module isn't perfect. (prometheus/prometheus#8852), so we'll need to think of another way to install promtool here. Maybe just download it from prometheus releases and add to our workspace image

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

2 similar comments
@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@meysholdt
Copy link
Member

I'll clarify under which license we want to publish this.

should be MIT license. We use hat for the helm chart and the Terraform scripts as well. The intention is that users of Gitpod self-hosted should be able to customise this code without worrying about licenses.

@csweichel
Copy link
Contributor

I'll clarify under which license we want to publish this.

should be MIT license. We use hat for the helm chart and the Terraform scripts as well. The intention is that users of Gitpod self-hosted should be able to customise this code without worrying about licenses.

That will require some minor changes to the addlicense script and potentially the need to add the MIT license to the license header program - no big changes, just pointing them out.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

1 similar comment
@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@ArthurSens
Add jsonnet extension
8d52ca5 
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

2 similar comments
@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

This was referenced Sep 30, 2021
Closed
Closed
@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@ArthurSens ArthurSens marked this pull request as ready for review September 30, 2021 13:56
@ArthurSens ArthurSens mentioned this pull request Oct 4, 2021
Merged
@csweichel
Copy link
Contributor

csweichel commented Oct 5, 2021 •
edited by werft-gitpod-dev-com bot

/werft run

👍 started the job as gitpod-build-as-migrate-mixins.84

@meysholdt
Copy link
Member

meysholdt commented Oct 5, 2021 •
edited by werft-gitpod-dev-com bot

/werft run

👍 started the job as gitpod-build-as-migrate-mixins.85

meysholdt
meysholdt reviewed Oct 5, 2021
View reviewed changes
dev/image/Dockerfile Outdated Show resolved Hide resolved
@ArthurSens
Migrates Gitpod's mixin to this repository.
54a4bf0 
Aiming to simplify the end-to-end developer experience of creating/modifying Prometheus metrics until using them in Dashboards and Alerts in production.

Signed-off-by: ArthurSens <arthursens2005@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2021

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@ArthurSens
Add eexamples for adding recording rules and alerts
d56e31b 
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2021

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@meysholdt
Copy link
Member

/lgtm
/approve

Code changes LGTM. Grafana works for me, too.
I could not connect to https://prometheus-as-migrate-mixins.preview.gitpod-dev.com/graph, but I assume that problem is unrelated to this PR, because I don't see code changes that seem related.

So let's get this in and iterate on it. Thank you for the great work, @ArthurSens !

@roboquat roboquat added the lgtm label Oct 5, 2021
@roboquat
Copy link
Contributor

roboquat commented Oct 5, 2021

LGTM label has been added.

Git tree hash: ea68918816e972431823f39c8e55068b93f51684

@roboquat
Copy link
Contributor

roboquat commented Oct 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: meysholdt

Associated issue: #192

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

@ArthurSens
Copy link
Contributor Author

/hold cancel

@roboquat roboquat merged commit f3a621b into main Oct 5, 2021
@roboquat roboquat deleted the as/migrate-mixins branch October 5, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meysholdt meysholdt meysholdt left review comments

@mrsimonemms mrsimonemms Awaiting requested review from mrsimonemms

Assignees

@meysholdt meysholdt

Labels
approved release-note-none size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ArthurSens @meysholdt @csweichel @roboquat