Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels #2747

tanmay-db · 2023-09-29T12:54:26Z

Changes

Terraform never had logging configured correctly (used to log all levels irrespective of log level specified). The issue (api calls not being shown in DEBUG log level) surfaced because in the TF version bump to 1.20.9 which had Go SDK version bump to 0.10.0, the PR: databricks/databricks-sdk-go#426 introduced INFO as basic log level leading to debug level never being enabled even if it is passed through the environment variable.

Note: terraform apply does not show INFO level logs by default. For that TF_LOG=INFO needs to be passed which I think is the expected (correct) behaviour.

Tests

Tested Manually with using TF_LOG=DEBUG (TF_LOG=debug also works) env which works, not using the env doesn't show logs (correctly), using TF_LOG=TRACE shows right level (TRACE) logs.
Manually running TF_LOG=DEBUG terraform apply shows API calls,

2023-09-29T14:51:24.653+0200 [DEBUG] provider.terraform-provider-databricks: GET /api/2.1/unity-catalog/catalogs
< HTTP/2.0 200 OK
< {
<   "catalogs": [
<     {
<       "browse_only": false,
<       "catalog_type": "MANAGED_CATALOG",
<       "comment": "",

Added unit test
make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

codecov-commenter · 2023-09-29T12:58:20Z

Codecov Report

Merging #2747 (e30ec5a) into master (0eff50f) will decrease coverage by 0.14%.
The diff coverage is 13.04%.

@@            Coverage Diff             @@
##           master    #2747      +/-   ##
==========================================
- Coverage   86.68%   86.55%   -0.14%     
==========================================
  Files         147      148       +1     
  Lines       12832    12855      +23     
==========================================
+ Hits        11124    11127       +3     
- Misses       1129     1149      +20     
  Partials      579      579

Files	Coverage Δ
provider/provider.go	`94.18% <100.00%> (+0.03%)`	⬆️
logger/logger.go	`9.09% <9.09%> (ø)`

mgyucht

I think this is the right approach, but we need to fix format strings.

logger/logger.go

…abricks into tflog

tanmay-db · 2023-10-02T10:05:22Z

logger/logger.go

+	}
+}
+
+func convertToMap(args ...interface{}) map[string]interface{} {


Not sure if there is a better way to do this.

I think it should be fine since we are already formatting the format string and then passing it to tflog. This function makes the method signature valid (accepts map[string]interface{} instead of any that we use in Go SDK)

I don't think this is needed at all. Any extra parameters during logging are expected to be inserted into the format string. IMO, you should pass nil as the extra argument everywhere. As is, this is not very useful, it's just a list of the arguments to the format string.

logger/logger_test.go

mgyucht

Some nits, but we're getting closer.

mgyucht · 2023-10-02T11:16:43Z

logger/logger.go

+// if the logging is enabled based on level (which default to Info). This however isn't required here
+// since we use the tflog package which automatically reads from TF_LOG environment variable.


It isn't that this is not required. tflog simply does not expose what log level is supported for a logger. If it did, we would delegate to tflog.

mgyucht · 2023-10-02T11:20:12Z

logger/logger.go

+	}
+}
+
+func convertToMap(args ...interface{}) map[string]interface{} {


I don't think this is needed at all. Any extra parameters during logging are expected to be inserted into the format string. IMO, you should pass nil as the extra argument everywhere. As is, this is not very useful, it's just a list of the arguments to the format string.

logger/logger_test.go

alexott · 2023-10-02T12:59:04Z

@tanmay-db btw, it doesn't work for exporter :-(

…in DEBUG or lower log levels (#2747) * add logger * fix format strings * - * - * - * - * - * - * -

add logger

0e301cc

tanmay-db requested review from a team as code owners September 29, 2023 12:54

tanmay-db requested review from mgyucht and removed request for a team and mgyucht September 29, 2023 12:54

tanmay-db changed the title ~~Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels~~ [WIP] Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels Sep 29, 2023

tanmay-db mentioned this pull request Sep 29, 2023

Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels. #2706

Closed

5 tasks

tanmay-db removed the request for review from a team September 29, 2023 12:55

mgyucht reviewed Sep 29, 2023

View reviewed changes

logger/logger.go Outdated Show resolved Hide resolved

logger/logger.go Outdated Show resolved Hide resolved

tanmay-db added 3 commits October 2, 2023 11:58

Merge branch 'master' of github.com:databricks/terraform-provider-dat…

29409ea

…abricks into tflog

fix format strings

07a837e

-

c0e16b1

tanmay-db commented Oct 2, 2023

View reviewed changes

tanmay-db requested a review from mgyucht October 2, 2023 10:05

tanmay-db changed the title ~~[WIP] Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels~~ Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels Oct 2, 2023

tanmay-db added 4 commits October 2, 2023 12:27

-

abf0737

-

4c8101d

-

7164498

-

ecef586

tanmay-db commented Oct 2, 2023

View reviewed changes

logger/logger_test.go Show resolved Hide resolved

mgyucht reviewed Oct 2, 2023

View reviewed changes

-

cc3e0e7

tanmay-db requested a review from mgyucht October 2, 2023 12:18

-

e30ec5a

mgyucht approved these changes Oct 2, 2023

View reviewed changes

tanmay-db added this pull request to the merge queue Oct 2, 2023

tanmay-db changed the title ~~Added logging package and fixed issue with API calls not being shown in DEBUG or lower log levels~~ Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels Oct 2, 2023

Merged via the queue into databricks:master with commit 9ef8366 Oct 2, 2023
4 checks passed

tanmay-db deleted the tflog branch October 2, 2023 12:36

nkvuong pushed a commit that referenced this pull request Oct 3, 2023

Added logging package and fixed issue with API calls not being shown …

8ac6749

…in DEBUG or lower log levels (#2747) * add logger * fix format strings * - * - * - * - * - * - * -

mgyucht mentioned this pull request Oct 4, 2023

Export: parallel export of resources #2742

Merged

9 tasks

tanmay-db mentioned this pull request Oct 12, 2023

Release v1.28.0 #2793

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels #2747

Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels #2747

tanmay-db commented Sep 29, 2023 •

edited

Loading

codecov-commenter commented Sep 29, 2023 •

edited

Loading

mgyucht left a comment

tanmay-db Oct 2, 2023

tanmay-db Oct 2, 2023 •

edited

Loading

mgyucht Oct 2, 2023

mgyucht left a comment

mgyucht Oct 2, 2023

mgyucht Oct 2, 2023

alexott commented Oct 2, 2023

		// if the logging is enabled based on level (which default to Info). This however isn't required here
		// since we use the tflog package which automatically reads from TF_LOG environment variable.

Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels #2747

Added logger package and fixed issue with API calls not being shown in DEBUG or lower log levels #2747

Conversation

tanmay-db commented Sep 29, 2023 • edited Loading

Changes

Tests

codecov-commenter commented Sep 29, 2023 • edited Loading

Codecov Report

mgyucht left a comment

Choose a reason for hiding this comment

tanmay-db Oct 2, 2023

Choose a reason for hiding this comment

tanmay-db Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

mgyucht Oct 2, 2023

Choose a reason for hiding this comment

mgyucht left a comment

Choose a reason for hiding this comment

mgyucht Oct 2, 2023

Choose a reason for hiding this comment

mgyucht Oct 2, 2023

Choose a reason for hiding this comment

alexott commented Oct 2, 2023

tanmay-db commented Sep 29, 2023 •

edited

Loading

codecov-commenter commented Sep 29, 2023 •

edited

Loading

tanmay-db Oct 2, 2023 •

edited

Loading