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

feat: Log resource when exception occurs to aid debugging #172

Merged
merged 2 commits into from
May 17, 2024

Conversation

hermanschaaf
Copy link
Member

@hermanschaaf hermanschaaf commented May 17, 2024

This change will allow plugin authors to see the resource that caused the exception, helping them find the underlying cause. To get this level of information, --log-level debug needs to be passed to the plugin server (or the CLI, depending on whether or not the plugin is run using the grpc registry)

Closes cloudquery/cloudquery#18002

@hermanschaaf hermanschaaf requested a review from a team as a code owner May 17, 2024 15:01
@hermanschaaf hermanschaaf requested review from bbernays and removed request for a team May 17, 2024 15:01
@@ -113,6 +113,7 @@ def resolve_table(
"failed to resolve resource",
client_id=client.id(),
table=resolver.table.name,
resource=item,

Choose a reason for hiding this comment

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

Can't this leak sensitive information? Is that something we are alright with?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, and think the chances of this happening are low enough that we can accept the risk/reward trade-off for now. This will help plugin authors during development, and should be relatively rare in production.

I'm open to other suggestions. Maybe only log it in verbose mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think logging it only in debug mode is better, so I've updated it now, PTAL 🙇

Comment on lines +119 to +124
self._logger.debug(
"details about resource that failed to resolve",
client_id=client.id(),
table=resolver.table.name,
resource=item,
)

Choose a reason for hiding this comment

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

I think this is a better approach that balances user experience with security

@hermanschaaf hermanschaaf added the automerge Add to automerge PRs once requirements are met label May 17, 2024
@kodiakhq kodiakhq bot merged commit 6403290 into main May 17, 2024
7 checks passed
@kodiakhq kodiakhq bot deleted the log-resource-on-exception branch May 17, 2024 15:30
kodiakhq bot pushed a commit that referenced this pull request May 17, 2024
🤖 I have created a release *beep* *boop*
---


## [0.1.23](v0.1.22...v0.1.23) (2024-05-17)


### Features

* Log resource when exception occurs to aid debugging ([#172](#172)) ([6403290](6403290))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to automerge PRs once requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Log failed message on column resolver exception in Plugin SDK
2 participants