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

feat(job-runner): enable snuba jobs command to run from JSON manifest #6288

Closed
wants to merge 4 commits into from
Closed

feat(job-runner): enable snuba jobs command to run from JSON manifest #6288

wants to merge 4 commits into from

Conversation

xurui-c
Copy link
Member

@xurui-c xurui-c commented Sep 11, 2024 •
edited
Loading

https://getsentry.atlassian.net/browse/SNS-2872

The SnS engineer will make JSON manifests in the ops repo. This PR enables us to pass in the file path of a JSON manifest file to the Snuba Jobs command

Next step will be validation within ManifestReader

Screenshot 2024-09-16 at 4 01 41 PM

@xurui-c xurui-c changed the title c Snuba Jobs command now reads JSON manifest Sep 11, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 11, 2024 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
584 1 583 1
View the top 1 failed tests by shortest run time 
tests.cli.test_jobs test_json_valid
Stack Traces | 0.002s run time 
Traceback (most recent call last):
  File ".../tests/cli/test_jobs.py", line 71, in test_json_valid
    assert result.exit_code == 0
AssertionError: assert 1 == 0
 +  where 1 = <Result FileNotFoundError(2, 'No such file or directory')>.exit_code

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@xurui-c xurui-c changed the title Snuba Jobs command now reads JSON manifest feat(job-runner): Snuba Jobs command now reads JSON manifest Sep 12, 2024
@xurui-c xurui-c force-pushed the rachel/json branch 5 times, most recently from 3865542 to 0d1f3b7 Compare September 16, 2024 21:12
xurui-c
xurui-c commented Sep 16, 2024
View reviewed changes
snuba/cli/jobs.py Outdated
def run(*, job_name: str, dry_run: bool, pairs: Tuple[str, ...]) -> None:
def run_from_manifest(*, json_manifest: str, job_id: str, dry_run: bool) -> None:
job_specs = ManifestReader.read(json_manifest)
for job_spec in job_specs:
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe ManifestReader.read can return a map of job id to job spec

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with that

xurui-c
xurui-c commented Sep 16, 2024
View reviewed changes
snuba/manual_jobs/job_loader.py
if job_type_class is None:
raise Exception(
f"Job does not exist. Did you make a file {class_name}.py yet?"
f"Job does not exist. Did you make a file {job_spec.job_type}.py yet?"
Copy link
Member Author

Choose a reason for hiding this comment

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

will make a snake case to camel case later

onewland
onewland reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@onewland onewland 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 this is looking good so far. Is it meant to be a draft PR?

snuba/manual_jobs/manifest_reader.py Outdated
for content in contents:
content_dict = dict(zip(content[::2], content[1::2]))
job_spec = JobSpec(
job_id=typing.cast(str, content_dict.get("id")),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make sense to do typing.assert_type here so we reject bad input (instead of just satisfying mypy) https://docs.python.org/3/library/typing.html#typing.assert_type

snuba/cli/jobs.py Outdated
def run(*, job_name: str, dry_run: bool, pairs: Tuple[str, ...]) -> None:
def run_from_manifest(*, json_manifest: str, job_id: str, dry_run: bool) -> None:
job_specs = ManifestReader.read(json_manifest)
for job_spec in job_specs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with that

@onewland onewland changed the title feat(job-runner): Snuba Jobs command now reads JSON manifest feat(job-runner): enable snuba jobs command to run from JSON manifest Sep 16, 2024
@xurui-c xurui-c marked this pull request as ready for review September 16, 2024 21:33
@xurui-c xurui-c requested a review from a team as a code owner September 16, 2024 21:33
onewland
onewland approved these changes Sep 16, 2024
View reviewed changes
snuba/manual_jobs/toy_job.py Outdated
@@ -16,4 +18,10 @@ def _build_query(self) -> str:
return "not dry run query"

def execute(self) -> None:
click.echo("executing query `" + self._build_query() + "`")
click.echo(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit that doesn't block merge: we shouldn't use click anything in this namespace because that's specific to CLI usage, and in prod we expect this to be invoked by snuba admin (let's logger.info in a later PR)

snuba/manual_jobs/manifest_reader.py Outdated

class _ManifestReader:

class ManifestReader:
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason that you want to make ManifestReader "public"? I'd prefer to expose as little as possible.

not a blocker for merge

@xurui-c xurui-c force-pushed the rachel/json branch 3 times, most recently from 1ab0b23 to 63a3e47 Compare September 17, 2024 18:24
Rachel Chen added 2 commits September 17, 2024 11:33