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

Add Doppler integration (ECOINT-6) #2460

Merged
merged 28 commits into from
Oct 10, 2024

Conversation

nmanoogian
Copy link
Contributor

What does this PR do?

This PR creates the Doppler log collection integration. This is an API-based integration that uses OAuth to connect Doppler with Datadog. Once connected, Doppler will begin sending activity log data to Datadog.

Motivation

What inspired you to submit this pull request?

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests (Not Relevant)
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If this PR includes a log pipeline, please add a description describing the remappers and processors.

@neko-dd
Copy link
Contributor

neko-dd commented Aug 6, 2024

Created DOCS-8635 for docs review

Copy link

@michaelcretzman michaelcretzman left a comment

Choose a reason for hiding this comment

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

Approved with some minor edits.

doppler/README.md Show resolved Hide resolved
doppler/README.md Show resolved Hide resolved
doppler/README.md Show resolved Hide resolved
@dd-dominic dd-dominic changed the title Add Doppler integration Add Doppler integration (ECOINT-6) Aug 23, 2024
doppler/README.md Show resolved Hide resolved
doppler/README.md Show resolved Hide resolved
doppler/assets/logs/doppler_tests.yaml Outdated Show resolved Hide resolved
doppler/assets/oauth_clients.json Outdated Show resolved Hide resolved
doppler/manifest.json Outdated Show resolved Hide resolved
@nmanoogian
Copy link
Contributor Author

Thanks all for the feedback!

A question for you: Do I need the files added in the "WIP: Add tests" commit? I re-ran ddev create and noticed that these came up.

I still need to let CI run and collect the test log results. I'll push again once that's done.

@nmanoogian
Copy link
Contributor Author

Another question: What should I provide for these manifest values if our integration doesn't produce metrics or provide service checks:

{
  "assets": {
    "integration": {
      "metrics": {
        "prefix": [
          "Missing data for required field."
        ],
        "metadata_path": [
          "Missing data for required field."
        ]
      },
      "service_checks": {
        "metadata_path": [
          "Missing data for required field."
        ]
      }
    }
  }
}

@emarsha94 emarsha94 requested review from a team and london-wharton and removed request for a team October 2, 2024 14:28
Copy link
Contributor

@london-wharton london-wharton left a comment

Choose a reason for hiding this comment

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

Left a suggestion to update the Oauth client id. I believe this should fix the failing validations!

doppler/assets/oauth_clients.json Outdated Show resolved Hide resolved
Copy link
Contributor

@london-wharton london-wharton left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for making that change!

doppler/manifest.json Outdated Show resolved Hide resolved
Change Author name to Partner name. This is what shows on the tile so it should be the integration name
@thibaultkrebs
Copy link
Contributor

I have check in our test environment for logs files. You can find below 2 screenshots in our test env.

I will approve the PR as it is. But before merging, you can consider maybe one improvement:
You could create more remapper from user to usr properties, so it avoid having both user with 2 properties and usr with 2 properties. All can be remapped to usr.
image

Generated samples:
image
image

@nmanoogian
Copy link
Contributor Author

I will approve the PR as it is. But before merging, you can consider maybe one improvement: You could create more remapper from user to usr properties, so it avoid having both user with 2 properties and usr with 2 properties. All can be remapped to usr

Good catch! I added remappers for those other two properties.

@london-wharton london-wharton merged commit d493466 into DataDog:master Oct 10, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent/approved assets/deploy-logs-staging ONLY USED BY Logs Backend - Validates that a PR is OK to go to staging docs/approved ecosystems/approved editorial review Waiting on a more in-depth review from a docs team editor kind/technology-partner product/approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants