Skip to content

Conversation

@ljbade
Copy link

@ljbade ljbade commented Mar 25, 2022

Our fork of prometheus-cpp is fairly out of date and I want to use some of the fixes and improvements from upstream.

I also fixed an unused variable error that shows up under latest upstream CI pipeline in the expired metrics code.

Finally I also fixed up bunch of missing/incorrect headers that show up with include-what-you-use which upstream enforces.

As for what to review, basically only the last three commits that are different from upstream:

  • Add support for expiring metrics
  • Run IWYU and fix most issues
  • Fix missing std:: on size_t
  • Fix reference alignment formatting

@ljbade ljbade requested a review from jbangelo March 25, 2022 01:25
@ljbade ljbade force-pushed the ljbade/rebase-upstream branch from 8ffb004 to ff45002 Compare March 25, 2022 01:31
@ljbade ljbade mentioned this pull request Mar 25, 2022
@ljbade ljbade force-pushed the ljbade/rebase-upstream branch from ff45002 to 2c66f4b Compare March 25, 2022 03:46
@ljbade
Copy link
Author

ljbade commented Mar 25, 2022

I removed the commit "Override labels with constant labels." because it turns out upstream has already fixed this issue on duplicate labels here - https://github.com/jupp0r/prometheus-cpp/blob/master/core/src/family.cc#L50

@ljbade
Copy link
Author

ljbade commented Mar 28, 2022

I had an annoying issue with the other PRs where clang-format wasn't correctly putting the & symbols aligned left on variable definitions. Was ending up with mix of left aligned (what everything in the project uses), and right aligned which is what we use in our own code. So set the clang-format file to avoid this.

@ljbade ljbade force-pushed the ljbade/rebase-upstream branch from 6837162 to dd539fd Compare March 28, 2022 22:42
@ljbade ljbade requested a review from silverjam March 28, 2022 23:08
@silverjam
Copy link

silverjam commented Mar 29, 2022

  • Add support for expiring metrics

Have we tried to upstream this feature? This seems pretty self-contained and easy to push upstream.

  • Run IWYU and fix most issues

Is this something that we can try to upstream too? (Does prometheus-cpp already use IWYU?) I'm wondering if applying IWYU is worth the overhead of making our fork drift more from upstream.

@silverjam
Copy link

  • Fix missing std:: on size_t
  • Fix reference alignment formatting

These also seem like things that we could upstream easily.

Copy link

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

I think we should try to upstream most of these changes and avoid the ones that cause drift from upstream without a large amount of material benefit. So we could potentially drop IWYU and the clang format changes.

@silverjam
Copy link

silverjam commented Mar 29, 2022

I think we should try to upstream most of these changes and avoid the ones that cause drift from upstream without a large amount of material benefit. So we could potentially drop IWYU and the clang format changes.

Nevermind, looks like prometheus-cpp already uses IWYU so we can submit a PR for that too. We can probably make a case for the clang-format change too.

@silverjam silverjam requested a review from notoriaga March 29, 2022 22:21
@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

@silverjam Good point, I'll drop those minor commits and make PRs for upstream (I'll make a ticket to do this once have bit of time).

The IWYU was originally so that I could make sure the expired metrics stuff had the correct headers, so I'll keep only the stuff directly related to that.

@ljbade ljbade force-pushed the ljbade/rebase-upstream branch 2 times, most recently from b389800 to 3fc5e1a Compare March 30, 2022 01:01
@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

I kept only the expired changes, and rolled one of the <ctime> includes IWYU picked up into it. I also rebased again on upstream master as they have fixed the Windows 2016 CI failure.

@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

I opened up some upstream PRs for the smaller changes that I removed from here.
I will now set this to be master, and will back up the old fork at old-master in case for some reason we still need the old commits.

@ljbade ljbade merged commit 3fc5e1a into master Mar 30, 2022
@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

Master has been updated so closing the PR.

@ljbade ljbade deleted the ljbade/rebase-upstream branch March 30, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants