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

[MM-60371] Update Pyroscope to v1 #793

Merged
merged 3 commits into from
Sep 4, 2024
Merged

[MM-60371] Update Pyroscope to v1 #793

merged 3 commits into from
Sep 4, 2024

Conversation

streamer45
Copy link
Contributor

Summary

PR adds support for Pyroscope v1 (specifically v1.7.1). We were still using the 0.x release which wasn't yet part of the larger Grafana ecosystem. A couple of things have changed since then, especially on the scraping front which is now a task delegated to a separate service (Grafana Alloy).

We also add support for agent goroutines profiling (which we weren't exposing) and block profiles.

There's still an open question of whether we should be using godeltaprof as an improvement but that would require changes to the server code as well so I am deferring that for the time being and stick to the stock profiles.

image

Ticket Link

https://mattermost.atlassian.net/browse/MM-60371

Fixes https://mattermost.atlassian.net/browse/MM-57545

Add support for goroutine and block profiles
@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Sep 2, 2024
@streamer45 streamer45 self-assigned this Sep 2, 2024
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

I had a deployment lying around, switched to this branch and re-run deployment create. I got this error:

Error: failed to create terraform env: error setting up metrics server: error upload alloy config: output: bash: line 1: /etc/alloy/config.alloy: No such file or directory
, error: Process exited with status 1
exit status 1

@streamer45
Copy link
Contributor Author

I had a deployment lying around, switched to this branch and re-run deployment create. I got this error:

Error: failed to create terraform env: error setting up metrics server: error upload alloy config: output: bash: line 1: /etc/alloy/config.alloy: No such file or directory
, error: Process exited with status 1
exit status 1

@agarciamontoro Well, I suppose you didn't go through terraform provisioning (running the metrics.sh script) since it existed already.

@agarciamontoro
Copy link
Member

@agarciamontoro Well, I suppose you didn't go through terraform provisioning (running the metrics.sh script) since it existed already.

Hmmmm, that may be the case :( I'd say that's a bug (just not from this PR). Is this something that Ansible would solve?

@agarciamontoro
Copy link
Member

I'll destroy and recreate to make sure it works on my end :)

@streamer45
Copy link
Contributor Author

@agarciamontoro Well, I suppose you didn't go through terraform provisioning (running the metrics.sh script) since it existed already.

Hmmmm, that may be the case :( I'd say that's a bug (just not from this PR). Is this something that Ansible would solve?

Well, even terraform could solve but we'd have to taint the instance and probably cause a full recreation. But yeah, something like Ansible would likely check the diff in the configuration state and apply the necessary changes.

@agarciamontoro
Copy link
Member

Well, even terraform could solve but we'd have to taint the instance and probably cause a full recreation. But yeah, something like Ansible would likely check the diff in the configuration state and apply the necessary changes.

Yeah, I'd love for the system to do the minimal changes needed; i.e., use the existing instance, apply just the config changes.

@streamer45
Copy link
Contributor Author

Well, even terraform could solve but we'd have to taint the instance and probably cause a full recreation. But yeah, something like Ansible would likely check the diff in the configuration state and apply the necessary changes.

Yeah, I'd love for the system to do the minimal changes needed; i.e., use the existing instance, apply just the config changes.

You need to design for it then :p

Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

It worked! But do we need both the instance and the service_name tags?
image
image
Can we keep just one?

@agarciamontoro
Copy link
Member

Those are also duplicated in the application side of things, right?
image

@streamer45
Copy link
Contributor Author

@agarciamontoro Not sure, to be honest, as it sounds both are required at config level:

https://grafana.com/docs/alloy/latest/reference/components/pyroscope/pyroscope.scrape/#targets-argument

May need to check if we can "hide" one.

@agarciamontoro
Copy link
Member

@streamer45, I guess that what would be better would be to have just agents and mattermost applications, and then filter by their tags. But do we need an application for each instance? I think that's the main difference with what we had before.

@streamer45
Copy link
Contributor Author

@streamer45, I guess that what would be better would be to have just agents and mattermost applications, and then filter by their tags. But do we need an application for each instance? I think that's the main difference with what we had before.

Okay, let me try to unify the service name at least since we probably don't need a separate one per instance.

@streamer45
Copy link
Contributor Author

streamer45 commented Sep 4, 2024

@agarciamontoro Done. This means you now need to select the additional instance tag to get profiles from a specific instance.

image

To keep it simple I am using the prefixes to generate the service name. I think it's clear enough but let me know if you'd rather have a different mapping (e.g. mattermost instead of app).

Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Looks good! It seems one of the new tests is failing, but approving to unblock :)

@streamer45 streamer45 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Sep 4, 2024
@streamer45 streamer45 merged commit 5f663ee into master Sep 4, 2024
1 check passed
@streamer45 streamer45 deleted the MM-60371 branch September 4, 2024 20:08
fmartingr pushed a commit that referenced this pull request Sep 17, 2024
* Update Pyroscope v1
Add support for goroutine and block profiles

* Unify service name for targets

* Update test
fmartingr pushed a commit that referenced this pull request Nov 6, 2024
* Update Pyroscope v1
Add support for goroutine and block profiles

* Unify service name for targets

* Update test
fmartingr pushed a commit that referenced this pull request Nov 7, 2024
* Update Pyroscope v1
Add support for goroutine and block profiles

* Unify service name for targets

* Update test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants