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 Cloudwatch Logs Insights Target #423

Merged

Conversation

Puneeth-n
Copy link
Contributor

What does this do?

Introduces the Cloudwatch Logs Insights Target

Why is it a good idea?

We can visualise both Cloudwatch metrics and logs within the same dashboard

Context

Questions

@Puneeth-n Puneeth-n force-pushed the feature/cloudwatch-logs-insights-target branch 2 times, most recently from 015f728 to 8a96565 Compare October 29, 2021 11:27
@Puneeth-n Puneeth-n marked this pull request as ready for review October 29, 2021 11:34
@Puneeth-n Puneeth-n force-pushed the feature/cloudwatch-logs-insights-target branch from 8a96565 to 61255e2 Compare October 29, 2021 11:36
Copy link
Collaborator

@JamesGibo JamesGibo left a comment

Choose a reason for hiding this comment

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

LGTM.
Just spotted that in the previous PR you removed the s from wrapLogMessages to wrapLogMessage, was this intentional?

Also if you have any example dashboards use the cloudwatch target that you would like to contribute to the repo to help people starting with the library, then please feel free to raise a separate PR
https://github.com/weaveworks/grafanalib/tree/main/grafanalib/tests/examples

@Puneeth-n Puneeth-n force-pushed the feature/cloudwatch-logs-insights-target branch from 470597f to 7ebf4c5 Compare October 29, 2021 12:24
@Puneeth-n
Copy link
Contributor Author

LGTM. Just spotted that in the previous PR you removed the s from wrapLogMessages to wrapLogMessage, was this intentional?

Also if you have any example dashboards use the cloudwatch target that you would like to contribute to the repo to help people starting with the library, then please feel free to raise a separate PR https://github.com/weaveworks/grafanalib/tree/main/grafanalib/tests/examples

This is how the dashboard is represented in the JSON when it is exported from Grafana:

    {
      "datasource": "cloudwatch",
      "gridPos": {
        "h": 9,
        "w": 24,
        "x": 0,
        "y": 15
      },
      "id": 2,
      "options": {
        "dedupStrategy": "none",
        "enableLogDetails": true,
        "prettifyLogMessage": true,
        "showCommonLabels": false,
        "showLabels": false,
        "showTime": true,
        "sortOrder": "Descending",
        "wrapLogMessage": true
      },
      "targets": [
        {
          "expression": "fields @timestamp, @xrayTraceId, @message | filter @message like /^(?!.*(START|END|REPORT|LOGS|EXTENSION)).*$/ | sort @timestamp desc",
          "id": "",
          "logGroupNames": [
            "/aws/lambda/l-invoice-collection-invoicing-prod"
          ],
          "namespace": "",
          "queryMode": "Logs",
          "refId": "A",
          "region": "default",
          "statsGroups": []
        }
      ],
      "title": "Logs",
      "transparent": true,
      "type": "logs"
    }

@JamesGibo
Copy link
Collaborator

OK, that make sense.
Might be worth keeping the variable name wrapLogMessages, to maintains compatibility with peoples dashboards?
Also happy for it to be changed as a minor change, if you remove the 's', please could you update the the doc string to remove the 's' as well?

:param wrapLogMessages: Toggle line wrapping

Thanks

@Puneeth-n
Copy link
Contributor Author

@JamesGibo I have reverted to wrapLogMessages in #424

@Puneeth-n
Copy link
Contributor Author

Puneeth-n commented Oct 29, 2021

@JamesGibo May be worth checking out? ;)

@JamesGibo
Copy link
Collaborator

@JamesGibo May be worth checking out? ;)

Interesting, thanks for sharing, will take a look.
If you have any feedback on grafanalib let me know by raising an issue or chatting on the Slack channel in the README.

@JamesGibo
Copy link
Collaborator

@JamesGibo I have reverted to wrapLogMessages in #424

Thanks once PR #424 is merged, this one will need to be updated to use the correct name

grafanalib/tests/test_cloudwatch.py Outdated Show resolved Hide resolved
@Puneeth-n Puneeth-n force-pushed the feature/cloudwatch-logs-insights-target branch from 7ebf4c5 to e18fb6e Compare October 29, 2021 16:08
@JamesGibo JamesGibo merged commit b7e8e0f into weaveworks:main Nov 1, 2021
@Puneeth-n Puneeth-n deleted the feature/cloudwatch-logs-insights-target branch November 1, 2021 10:46
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.

None yet

2 participants