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.

Sign up for GitHub

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

Jump to bottom

Plugin Framework: propagate logging output to Pulumi #891

Merged
merged 20 commits into from Mar 21, 2023
Merged

Plugin Framework: propagate logging output to Pulumi #891

merged 20 commits into from Mar 21, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 15, 2023 •
edited

Fixes #820

Dependencies (required upstream fixes to pass tests):

github.com/pulumi/pulumi/pkg/v3 v3.58.0
github.com/pulumi/pulumi/sdk/v3 v3.58.0

Currently this gives output that looks something like this through Pulumi CLI:

  random:index:RandomInteger (integer):
    2023-03-15T15:08:51.429-0500  provider/resource_integer.go:100: provider: Create RandomInteger - INFO: provider=random providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty" superfield=supervalue
    warning: 2023-03-15T15:08:51.430-0500  provider/resource_integer.go:101: provider: Create RandomInteger - WARN: provider=random providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty" superfield=supervalue
    error: 2023-03-15T15:08:51.430-0500 provider/resource_integer.go:102: provider: Create RandomInteger - ERROR: providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty" superfield=supervalue provider=random
    2023-03-15T15:08:51.430-0500  provider/resource_integer.go:111: provider: Create RandomInteger - INFO +fields: b=b a=1 provider=random providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty" superfield=supervalue
    warning: 2023-03-15T15:08:51.431-0500  provider/resource_integer.go:112: provider: Create RandomInteger - WARN +fields: superfield=supervalue a=1 b=b provider=random providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty"
    error: 2023-03-15T15:08:51.431-0500 provider/resource_integer.go:113: provider: Create RandomInteger - ERROR +fields: superfield=supervalue provider=random providerVersion="4.12.0-alpha.1678477401+fa71af56.dirty" a=1 b=b
  • What should be the default log level? Should it be INFO as in PR or should it be OFF by default until the user sets TF_LOG?
  • Should we include timestamps or drop?
  • Do we like source locators like provider/resource_integer.go:113 ? Seems somewhat useful for debugging
  • How about provider=random and providerVersion="..." - include or drop?
  • Any user-provided custom fields like a=1 should probably stay

t0yv0
t0yv0 commented Mar 15, 2023
View reviewed changes
pf/internal/logging/logging_test.go
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
)

func TestLogging(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the interesting new bit, new package to connect up logging and the tests thereof.

@t0yv0 t0yv0 requested review from iwahbe and a team March 15, 2023 21:11
@github-actions
Copy link

Diff for pulumi-azuread with merge commit df5ce23

@github-actions
Copy link

Diff for pulumi-random with merge commit df5ce23

@github-actions
Copy link

Diff for pulumi-gcp with merge commit df5ce23

@github-actions
Copy link

Diff for pulumi-azure with merge commit df5ce23

iwahbe
iwahbe approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

What should be the default log level? Should it be INFO as in PR or should it be OFF by default until the user sets TF_LOG?

Since we will be consuming logs generated by providers written for TF, I think we should emulate what TF does.

Should we include timestamps or drop?

I don't generally find them useful, but no strong feelings either way.

Do we like source locators like provider/resource_integer.go:113 ? Seems somewhat useful for debugging

Love it

How about provider=random and providerVersion="..." - include or drop?

I think we should include, but lets shorten them to the way the plugin loader logs this:

Replace

provider=random providerVersion="4.12.0 alpha.1678477401+fa71af56.dirty"

With

provider=random@4.12.0alpha.1678477401+fa71af56.dirty

Any user-provided custom fields like a=1 should probably stay

🆗

pf/internal/logging/logging.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Diff for pulumi-random with merge commit 20f4df3

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 20f4df3

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 20f4df3

@github-actions
Copy link

Diff for pulumi-azure with merge commit 20f4df3

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 8a29e12

@github-actions
Copy link

Diff for pulumi-random with merge commit 8a29e12

@github-actions
Copy link

Diff for pulumi-azuread with merge commit eba64ec

@github-actions
Copy link

Diff for pulumi-random with merge commit eba64ec

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 8a29e12

@github-actions
Copy link

Diff for pulumi-azure with merge commit eba64ec

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 28e449e

@github-actions
Copy link

Diff for pulumi-random with merge commit 28e449e

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 28e449e

@github-actions
Copy link

Diff for pulumi-azure with merge commit 28e449e

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 3452060

@github-actions
Copy link

Diff for pulumi-random with merge commit 3452060

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 3452060

@github-actions
Copy link

Diff for pulumi-azure with merge commit 3452060

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 21, 2023

So it turns out if one follows instructions from Pulumi troubleshooting page, the logs are written out to a file already, and Pulumi adds timestamps, so that's great. I removed timestamps at TF level. Addressed the other bits. Will merge once I get the tests to pass.

@github-actions
Copy link

Diff for pulumi-random with merge commit 0536183

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 0536183

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 0536183

@github-actions
Copy link

Diff for pulumi-azure with merge commit 0536183

@t0yv0 t0yv0 merged commit 447b802 into master Mar 21, 2023
7 checks passed
@t0yv0 t0yv0 deleted the t0yv0/pf-logging branch March 21, 2023 21:06
t0yv0 added a commit that referenced this pull request Mar 28, 2023
@t0yv0
Restores accidentally changed signature of pf/tfbridge NewProvider
9ee6679 
A recent PR #891 has accidentally changed the signature of an exported func NewProvider in the pf/tfbridge package. This
function is used in tests including tests of pulumi-random provider and should not change. The original signature is now
restored.
@t0yv0 t0yv0 mentioned this pull request Mar 28, 2023
Merged
t0yv0 added a commit that referenced this pull request Mar 28, 2023
@t0yv0
Restores accidentally changed signature of pf/tfbridge NewProvider
6f23dc6 
A recent PR #891 has accidentally changed the signature of an exported func NewProvider in the pf/tfbridge package. This
function is used in tests including tests of pulumi-random provider and should not change. The original signature is now
restored.
t0yv0 added a commit that referenced this pull request Mar 28, 2023
@t0yv0
Restores accidentally changed signature of pf/tfbridge NewProvider (#921
4806fbe 
)

A recent PR #891 has accidentally changed the signature of an exported func NewProvider in the pf/tfbridge package. This function is used in tests including tests of pulumi-random provider and should not change. The original signature is now restored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwahbe iwahbe iwahbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Plugin Framework] Init and connect Terraform logging to Pulumi logging
2 participants
@t0yv0 @iwahbe