-
Notifications
You must be signed in to change notification settings - Fork 784
[ECOINT-154] Add Dagster+ integration #2655
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions, let me know if you have any questions on my comments!
dagster/README.md
Outdated
For any issues, visit [our support page.][1] | ||
|
||
|
||
[1]: https://dagster.io/support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[1]: https://dagster.io/support | |
[1]: https://docs.dagster.io/guides/monitor/logging | |
[2]: https://app.datadoghq.com/organization-settings/api-keys | |
[3]: https://dagster.io/support | |
dagster/README.md
Outdated
|
||
### Validation | ||
|
||
Within the next ten minutes, the Dagster Overview Dashboard will begin showing new log events if there are any active Dagster jobs emitting events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within the next ten minutes, the Dagster Overview Dashboard will begin showing new log events if there are any active Dagster jobs emitting events. | |
Within 10 minutes of completing the integration setup, the Dagster Overview Dashboard starts showing new log events, provided there are any active Dagster jobs emitting events. |
@estherk15 ready for another review, thanks! |
dagster/README.md
Outdated
|
||
## Setup | ||
|
||
1. Click **Connect Accounts** to launch the OAuth flow and link your Dagster and Datadog accounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't catch this earlier, but where do you click from? Datadog integration tile or Dagster? If you have it, add a link here.
Review from estherk15 is dismissed. Related teams and files:
- documentation
- dagster/README.md
This pull request has not been updated for more than 21 days. If there are no updates to this PR within 7 days, it will be closed. If you'd like to re-open this PR after it's been closed, you can start from the latest master branch or pull the latest changes into your branch and create a new pull request. |
@@ -0,0 +1,60 @@ | |||
id: dagster-plus | |||
# See app_id in your integration's manifest.json file to learn more: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all these comments, they are just here to help you create the integration
- | ||
sample: |- | ||
{ | ||
"id" : "AQAAAZZFaQ9F2IkRTQAAAAAxNzY4OTkyNDIw", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample should be the RAW message you are sending to Datadog. Here you have provided the result we see in Datadog after we have processed your log. Nothing is getting tested. Can you replace with the proper original raw message?
Review from estherk15 is dismissed. Related teams and files:
- documentation
- dagster/README.md
- dagster/assets/dashboards/dagster__overview.json
- dagster/manifest.json
"run_id" : "a3bcb088-1640-40dc-9701-4b554c0b9cac", | ||
"event_type" : "STEP_WORKER_STARTED", | ||
"job_name" : "diamond_asset_job", | ||
"service" : "dagster-plus", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a service remapper? So it sets this as a core attribute
sourceType: attribute | ||
target: dagster-plus.event_type | ||
targetType: attribute | ||
preserveSource: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preserveSource should not be true. It will duplicate the property into two paths when it is not needed
metric_id: dagster-plus | ||
backend_only: false | ||
facets: | ||
- description: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can remove the empty description here
Integration Dagster+ has been created in publishing platform