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

Fix incorrectly applied "removed" status for pings #829

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

wlach
Copy link
Contributor

@wlach wlach commented Sep 13, 2021

Instead, we'll just say that they're always in source until we can
determine otherwise (see mozilla/probe-scraper#341)

@wlach
Copy link
Contributor Author

wlach commented Sep 13, 2021

This bug is very visible if you look at the pings page for Fenix:

https://dictionary.telemetry.mozilla.org/apps/fenix?itemType=pings&page=1

image

@wlach wlach requested a review from Iinh September 13, 2021 21:16
@wlach
Copy link
Contributor Author

wlach commented Sep 13, 2021

Yuck, this actually only half-fixes the problem since app_ids suffer from the same thing. I think the right solution here is to only consider an item removed if the in_source annotation is actually false. Not all items have the "in source" annotation. Will update PR.

@wlach wlach force-pushed the fix-removed-status-for-pings branch 3 times, most recently from 9185a12 to 9e26e55 Compare September 13, 2021 22:22
@wlach wlach requested a review from Iinh September 13, 2021 22:24
We'll say that an item is only removed if it (1) has an "in_source"
annotation and it is false. This annotation should always be present
for metrics, but is not currently set for any other object.
@wlach wlach force-pushed the fix-removed-status-for-pings branch from 9e26e55 to c281700 Compare September 13, 2021 22:31
return item.in_source === false;
};

export const isExpired = (item) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt like it would be best if these two methods took the same argument.

Copy link
Contributor

@Iinh Iinh 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 your patience, this looks good!

@wlach wlach merged commit 5a4f1bd into main Sep 14, 2021
@wlach wlach deleted the fix-removed-status-for-pings branch September 14, 2021 18:31
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.

2 participants