Skip to content

ref: Use defaults for start and end times to Snuba#14289

Merged
manuzope merged 2 commits into
masterfrom
SNS-192-improve-snuba-client-retention-querying-2
Aug 6, 2019
Merged

ref: Use defaults for start and end times to Snuba#14289
manuzope merged 2 commits into
masterfrom
SNS-192-improve-snuba-client-retention-querying-2

Conversation

@manuzope

@manuzope manuzope commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

We have the following logic in a few places:

snuba.raw_query(
    start=datetime.utcfromtimestamp(0),  # will be clamped to project retention
    end=datetime.utcnow(),  # will be clamped to project retention
    ...
)

However, we have some pretty reasonable defaults for start and end, so, let's use them.

@manuzope manuzope marked this pull request as ready for review August 5, 2019 21:32
@manuzope manuzope requested a review from a team August 5, 2019 21:32

@tkaemming tkaemming left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks — hard to believe nobody did this earlier. 😬

values_by_key = snuba.query(
start, end, ['tags_key', 'tags_value'], conditions, filters, aggregations,
kwargs.get('start'), kwargs.get('end'), [
'tags_key', 'tags_value'], conditions, filters, aggregations,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using kwargs like this is kind of an anti-pattern but since you didn't introduce it, it is probably better to remain consistent with the existing code and not increase the scope of the change, so this is okay. Mostly pointing this out to say please don't take this as an example of good coding practice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good to know! When I do want to extract arguments from kwargs, is it better to extract it in the params?

get_group_tag_keys_and_top_values(...start=None, end=None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes — for example in this case, if end=xyz was accidentally replaced with stop=xyz or some other invalid argument, the method would accept the invalid keyword argument, but not use it. This makes it difficult to identify incorrect usage, especially when refactoring

@manuzope manuzope merged commit 034dbb0 into master Aug 6, 2019
@manuzope manuzope deleted the SNS-192-improve-snuba-client-retention-querying-2 branch August 6, 2019 16:46
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 20, 2020
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.

2 participants