Skip to content
This repository has been archived by the owner on Jul 19, 2023. It is now read-only.

Add -modules support #497

Merged
merged 15 commits into from
Feb 21, 2023
Merged

Add -modules support #497

merged 15 commits into from
Feb 21, 2023

Conversation

Rustin170506
Copy link
Contributor

close #367

Try to add -modules support. Most code from mimir.

cmd/phlare/main.go Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Contributor Author

Rustin170506 commented Feb 6, 2023

@cyriltovena Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, this is super useful to have.

I only have one request, could add the existence of that flag to the -help output? Currently it is not mentioned at all. You could modify -target or also register a separate flag there (or maybe even both)

@Rustin170506
Copy link
Contributor Author

I only have one request, could add the existence of that flag to the -help output?

Of course. Also, I noticed that phlare's current help info has some problems. like it uses the wrong name.

phlare git:(main) ✗ ./main --help            
Usage of config-file-loader:
  -auth.multitenancy-enabled
        When set to true, incoming HTTP requests must specify tenant ID in HTTP X-Scope-OrgId header. When set to false, tenant ID anonymous is used instead.
  -client.tenant-id string
        Tenant ID to use when pushing profiles to Phlare (default: anonymous). (default "anonymous")
  -client.url value
        URL of log server.
  -config.expand-env
        Expands ${var} in config according to the values of the environment variables.

I'll fix it together.

@Rustin170506 Rustin170506 marked this pull request as draft February 7, 2023 03:01
@Rustin170506 Rustin170506 changed the title Add -modules support [WIP] Add -modules support Feb 7, 2023
@Rustin170506

This comment was marked as outdated.

@Rustin170506 Rustin170506 changed the title [WIP] Add -modules support Add -modules support Feb 8, 2023
@Rustin170506 Rustin170506 marked this pull request as ready for review February 8, 2023 13:28
cmd/phlare/main.go Outdated Show resolved Hide resolved
@Rustin170506

This comment was marked as outdated.

@Rustin170506

This comment was marked as outdated.

pkg/phlare/phlare.go Outdated Show resolved Hide resolved
@simonswine
Copy link
Collaborator

@simonswine Friendly ping~ Thank you!

@hi-rustin I am really sorry that it took so long. Keep pinging and eventually I will get to it 🙂.

I would prefer we change the structure of cmd/phlare.main to be more aligned with cmd/mimir.main, let me know if you don't think that is a good idea.

Copy link
Contributor Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Self check

@Rustin170506
Copy link
Contributor Author

Rustin170506 commented Feb 21, 2023

@simonswine
Thanks for reviewing! 💚 💙 💜 💛 ❤️

It is ready for another review.

Also, I think we still have some work to do.

  1. add a test for the CLI help text, then we can notice what has changed after every PR
  2. add a test mode to test some abnormal scenarios, like getting an unknown args. Now we cannot test it because we called os.Exit.

I can do it in my following PRs. Because it's off this topic.

Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Yes fully agree with solving those problems are off-topic to this PR, it is gotten large enough already.

Thank you so much for going through the review rounds. LGTM

@simonswine simonswine merged commit 3b273c4 into grafana:main Feb 21, 2023
@Rustin170506 Rustin170506 deleted the rustin-flags branch February 21, 2023 14:27
@Rustin170506
Copy link
Contributor Author

Thanks for your review! 💚 💙 💜 💛 ❤️

This was referenced Feb 22, 2023
simonswine pushed a commit to simonswine/pyroscope that referenced this pull request Jun 30, 2023
This adds the ability to list modules, display a basic flag `-help` and `-help-all` including all flags. And handles `-version` from the main package.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide -modules command
2 participants