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

Bug 1648696 - Look for metrics in send_if_empty pings #197

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Bug 1648696 - Look for metrics in send_if_empty pings #197

merged 3 commits into from
Jun 30, 2020

Conversation

brizental
Copy link
Contributor

@brizental brizental commented Jun 29, 2020 •
edited
Loading

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • make test runs without emitting any warnings
    • make lint runs without emitting any errors
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry to HISTORY.rst or an explanation of why it does not need one
    • Any breaking changes to language binding APIs are noted explicitly

@brizental
Copy link
Contributor Author

Hum, actually seems like this was just a problem I was having locally due to misconfiguration. I believe it might be coming from some old version of the glean parser, which I was using unintentionally.

@brizental brizental closed this Jun 29, 2020
@brizental brizental deleted the 1648696-docs-empty branch June 29, 2020 16:55
@brizental brizental restored the 1648696-docs-empty branch June 30, 2020 10:00
@brizental
Copy link
Contributor Author

brizental commented Jun 30, 2020 •
edited
Loading

Okay, yes this is an issue. I just made a big confusion where I had multiple glean_parser options installed locally, etc. Not even worth explaining here.

@brizental brizental reopened this Jun 30, 2020
@brizental brizental requested a review from Dexterp37 June 30, 2020 10:07
brizental
brizental commented Jun 30, 2020
View reviewed changes
Makefile
@@ -21,7 +21,7 @@ clean-build: ## remove build artifacts
rm -fr dist/
rm -fr .eggs/
find . -name '*.egg-info' -exec rm -fr {} +
find . -name '*.egg' -exec rm -f {} +
find . -name '*.egg' -exec rm -fr {} +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated. I ran into issues while running make clean

@Dexterp37 Dexterp37 removed their request for review June 30, 2020 11:11
@brizental brizental requested a review from Dexterp37 June 30, 2020 12:09
Dexterp37
Dexterp37 approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@Dexterp37 Dexterp37 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 to me. We should test how this will change both Fenix docs and the Glean SDK docs, if you didn't do that already.

tests/data/send_if_empty_with_metrics.yaml
type: uuid
lifetime: ping
description: >
Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Copy link
Contributor

Choose a reason for hiding this comment

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

The designer part of you is now surfacing :)

@brizental
Copy link
Contributor Author

brizental commented Jun 30, 2020 •
edited
Loading

We should test how this will change both Fenix docs and the Glean SDK docs, if you didn't do that already.

Neither fenix or android-components have send_if_empty pings. Their docs should be unchanged. Glean SDK same thing, but the docs created on this PR will be fixed.

@brizental brizental merged commit 3c8de75 into main Jun 30, 2020
@brizental brizental deleted the 1648696-docs-empty branch June 30, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dexterp37 Dexterp37 Dexterp37 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brizental @Dexterp37