Skip to content

Conversation

@bbirdsong2
Copy link
Contributor

This new job is to add a DAP collector for the ads eng team to test internally with the DAP infrastructure setup by the CTO office.

Checklist for reviewer:

  • Commits should reference a bug or github issue, if relevant (if a bug is
    referenced, the pull request should include the bug number in the title)
  • Scan the PR and verify that no changes (particularly to
    .circleci/config.yml) will cause environment variables (particularly
    credentials) to be exposed in test logs
  • Ensure the container image will be using permissions granted to
    telemetry-airflow
    responsibly.

@BenWu
Copy link
Contributor

BenWu commented Apr 29, 2024

I triggered CI by pushing to the add-dap-collector-ppa-dev-job branch but I'll let someone familiar with the business logic do a review

Copy link
Contributor

@quiiver quiiver left a comment

Choose a reason for hiding this comment

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

I'm a little worried about the pattern we're introducing here. The implication is that every unique use case of DAP would require its own docker-etl container. I feel like there are a couple of patterns that could provide a more generic or reusable etl of DAP tasks. For example we could have a generic DAP collector task that takes some configuration for tasks, auth, etc. Then as part of the airflow DAG we transform the collected reports into whatever final output we care about. I'm happy to talk through the options we have at our disposal if you'd like.

That said I don't see any blockers to landing this as is.

@bbirdsong2
Copy link
Contributor Author

@quiiver Yeah, I agree. DAP is still experimental so I am not currently bothered by the duplication, but once we decide to move forward with DAP I think we can better conslidate the infrastructure.

@quiiver quiiver merged commit 0ac787d into mozilla:main May 3, 2024
@bbirdsong2 bbirdsong2 deleted the add-dap-collector-ppa-dev-job branch May 3, 2024 19:54
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.

3 participants