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

Add Traefik Mesh integration #17585

Merged
merged 18 commits into from
May 30, 2024
Merged

Add Traefik Mesh integration #17585

merged 18 commits into from
May 30, 2024

Conversation

Kyle-Neale
Copy link
Contributor

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@datadog-agent-integrations-bot datadog-agent-integrations-bot bot added documentation release qa/skip-qa Automatically skip this PR for the next QA labels May 16, 2024
@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@Kyle-Neale Kyle-Neale added do-not-merge/WIP integration/traefik_mesh and removed release qa/skip-qa Automatically skip this PR for the next QA labels May 17, 2024
@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@Kyle-Neale Kyle-Neale marked this pull request as ready for review May 22, 2024 21:50
@Kyle-Neale Kyle-Neale requested review from a team as code owners May 22, 2024 21:50
@jhgilbert
Copy link
Contributor

Hi, I've created a docs review ticket, so the team can follow up when this is ready for a final editorial review. Thank you!

@jhgilbert jhgilbert added the editorial review Waiting on a more in-depth review from a docs team editor label May 23, 2024
alai97

This comment was marked as resolved.

Sign in to view

Kyle-Neale and others added 2 commits May 23, 2024 22:33
@Kyle-Neale @alai97
apply readme suggestions
01e1d1b 
Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>
@Kyle-Neale @alai97
apply suggestions to readme
a71a43a 
Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>
@Kyle-Neale Kyle-Neale changed the title Initial commit Add Traefik Mesh integration May 24, 2024
iliakur
iliakur reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

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

First impressions: one suggestion, one question.

traefik_mesh/datadog_checks/traefik_mesh/check.py
super().check(_)

finally:
if self.traefik_controller_api_endpoint:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation for submitting an extra service check? Is it cuz the extras integration had it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One service check gives us the health status of the Traefik Proxy /metrics endpoint. The other is a health status on the actually Mesh controller which can't be verified from the proxy. In general though, the metrics from the controller are only avialable if self.traefik_controller_api_endpoint is present.

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 I follow. Next question: does the order in which we submit checks matter here? Also, what do we gain by using try:...finally:? Could we also just do this?

            if self.traefik_controller_api_endpoint:
                self.submit_controller_readiness_service_check()
                traefik_node_status = self.get_mesh_ready_status()
                self.submit_mesh_ready_status(traefik_node_status)
           self._submit_version()
           super().check(_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea we could - I was simply using other integrations that do similar things as a template and that was what they do. If you have a strong opinion of using this way over the way it's currently written, I can definitely change.

Copy link
Contributor

Choose a reason for hiding this comment

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

nah 🤷

traefik_mesh/tests/test_e2e.py Outdated Show resolved Hide resolved
traefik_mesh/tests/test_unit.py Outdated Show resolved Hide resolved
@Kyle-Neale Kyle-Neale dismissed alai97’s stale review May 30, 2024 13:18

suggestions applied

iliakur
iliakur requested changes May 30, 2024
View reviewed changes
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

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

just a few minor touches left

traefik_mesh/tests/conftest.py Outdated Show resolved Hide resolved
traefik_mesh/tests/test_unit.py Outdated Show resolved Hide resolved
traefik_mesh/tests/test_unit.py Outdated Show resolved Hide resolved
iliakur
iliakur approved these changes May 30, 2024
View reviewed changes
traefik_mesh/datadog_checks/traefik_mesh/check.py
super().check(_)

finally:
if self.traefik_controller_api_endpoint:
Copy link
Contributor

Choose a reason for hiding this comment

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

nah 🤷

@Kyle-Neale Kyle-Neale merged commit 7fb0a90 into master May 30, 2024
44 checks passed
@Kyle-Neale Kyle-Neale deleted the kyleneale/traefik_mesh branch May 30, 2024 19:25
github-actions bot pushed a commit that referenced this pull request May 30, 2024
@Kyle-Neale
Add Traefik Mesh integration (#17585)
8d20f44 
* Initial commit

* Add unit and end to end tests

* added more test and assets to integration

* sync CI

* fix dashboard and E2E tests

* fix metadata types

* added more Traefik config metrics

* Add changelog and readme

* remove additional changelog added

* apply readme suggestions

Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>

* apply suggestions to readme

Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>

* added logo and fixed nits

* removed unneeded PY2 import and made tests more readable

---------

Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com> 7fb0a90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iliakur iliakur iliakur approved these changes

@steveny91 steveny91 steveny91 approved these changes

@alai97 alai97 alai97 left review comments

Assignees
No one assigned
Labels
dev/testing dev/tooling documentation editorial review Waiting on a more in-depth review from a docs team editor integration/traefik_mesh qa/skip-qa Automatically skip this PR for the next QA release team/agent-integrations team/documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Kyle-Neale @jhgilbert @iliakur @steveny91 @alai97