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

Add tools/metrics from Chromium 114.0.5735.330 #1031

Merged
merged 7 commits into from
Aug 15, 2023
Merged

Conversation

briantting
Copy link
Contributor

@briantting briantting commented Jul 25, 2023

b/292142280

@codecov

This comment was marked as spam.

Copy link
Contributor

@dahlstrom-g dahlstrom-g left a comment

Choose a reason for hiding this comment

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

The differences may have no functional consequence, but
commit 138a0c1 doesn't match the Chromium release:

$ git diff --shortstat 138a0c14 a281dd4e:tools/metrics
 4 files changed, 875 insertions(+), 3 deletions(-)

tools/metrics/METADATA Outdated Show resolved Hide resolved
@dahlstrom-g dahlstrom-g changed the title Create tools/metrics filtered subtree. Add tools/metrics from Chromium 114.0.5735.320 Aug 2, 2023
@dahlstrom-g dahlstrom-g changed the title Add tools/metrics from Chromium 114.0.5735.320 Add tools/metrics from Chromium 114.0.5735.329 Aug 9, 2023
Chrome Release Bot (LUCI) added 2 commits August 9, 2023 10:00
git-subtree-dir: tools/python
git-subtree-split: 4a8656d20e117967a89326daae7d5f88e0e8ebaa
git-subtree-dir: tools/metrics
git-subtree-split: 4a8656d20e117967a89326daae7d5f88e0e8ebaa
tools/metrics/METADATA Outdated Show resolved Hide resolved
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

If this is destined for 24lts then it can't go to Chrome 114, it needs to stay in sync with the rest of the language requirements and probably with where our components/metrics came from as well. Trunk is a separate matter and we can move faster there

@dahlstrom-g
Copy link
Contributor

I don't know if Joel would want this in 24.lts.1+; I suppose we can ask him next week.

@dahlstrom-g dahlstrom-g changed the title Add tools/metrics from Chromium 114.0.5735.329 Add tools/metrics from Chromium 114.0.5735.330 Aug 10, 2023
@joeltine
Copy link
Contributor

joeltine commented Aug 14, 2023

I'm a bit torn on the question of this going into the 24 lts branch. This directory by itself does not impact metrics logging inside the binary. It's just a set of tools for managing histogram/user action metadata and syncing them into Piper/google3.

In theory, using this directory at trunk should provide all the same benefits for someone working on the 24 branch. The only situation I could see being an issue is if someone only had access to the 24 branch outside of Google and wanted to poke around at the histogram metadata. If this use case isn't a concern, I think it just living at trunk is fine. @kaidokert thoughts?

@joeltine
Copy link
Contributor

it needs to stay in sync with the rest of the language requirements and probably with where our components/metrics came from as well.

Also, just want to add, the decision to sync this package to 114 was intentional. It's OK this is ahead of the rest of the metrics packages. As mentioned in the comment above, this is all about metric metadata management. I want the latest here. It's OK if we're missing some histograms in the metadata files as they already are written inside Google.

@briantting
Copy link
Contributor Author

briantting commented Aug 14, 2023

Can @dahlstrom-g repush again? I accidentally used this branch for another PR which wiped it. Sorry, I see what was happening.

dahlstrom-g and others added 5 commits August 14, 2023 18:00
… from tag 114.0.5735.330.
… for tools/metrics and tools/python.
… to satisfy uses in tools/metrics.
Change-Id: I026912b352f93bd1fb15e3142f140047b988f920
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4743547
Commit-Queue: Dana Dahlstrom <[email protected]>
Reviewed-by: Sun Yueru <[email protected]>
Reviewed-by: Robert Kaplow <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1179167}
… for compatibility with Python versions before 3.9, which lack PEP 585.

Change-Id: Ib747d709a8762bcf81e23ddbbf6a4ec4a2c4ce97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4762987
Commit-Queue: Dana Dahlstrom <[email protected]>
Reviewed-by: Weilun Shi <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1181199}
@kaidokert
Copy link
Member

If this use case isn't a concern, I think it just living at trunk is fine. @kaidokert thoughts?

That use case isn't a concern, and lets agree to keep it in trunk.

Another related question that if those are just tools - can't we use them directly from a Chrome checkout ? Fine either way, approving

@joeltine
Copy link
Contributor

joeltine commented Aug 15, 2023

Another related question that if those are just tools - can't we use them directly from a Chrome checkout ? Fine either way, approving

Nah because I'll need to make local modifications. For example, there will be a cobalt/histograms.xml file for the Cobalt specific histograms and all these scripts expect a local directory structure.

@kaidokert kaidokert merged commit 4f737ae into youtube:main Aug 15, 2023
330 of 331 checks passed
@joeltine
Copy link
Contributor

Thank you Brian and Dana for working to push this through!!

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.

5 participants