Skip to content

Replace deprecated logging param in nwo #571

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

Merged
merged 2 commits into from
May 23, 2024

Conversation

mbrandenburger
Copy link
Member

Currently, NWO uses the deprecated --logging-level param when invoking the peer cli. This commit replaces this param with FABRIC_LOGGING_SPEC env variable.

@adecaro adecaro self-requested a review May 21, 2024 04:25
Copy link
Contributor

@alexandrosfilios alexandrosfilios left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -550,7 +550,7 @@ func (p *Platform) fscNodeCommand(node *node2.Replica, command common.Command, t
cmd.Args = append(cmd.Args, "--keyfile", keyfilePath)
}

cmd.Args = append(cmd.Args, "--logging-level", p.Topology.Logging.Spec)
cmd.Env = append(cmd.Env, fmt.Sprintf("FABRIC_LOGGING_SPEC=%s", p.Topology.Logging.Spec))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this completely. This code is related to the execution of the FSC node.

Copy link
Member Author

Choose a reason for hiding this comment

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

don't we also use flogging within the FSC nodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we remove this, how do we pass the FSC node logging level when running it via nwo?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we do use the FSC's flogging. But the effect of setting FABRIC_LOGGING_SPEC is to have viper overridding the content of the key fabric.logging.spec. The FSC nodes uses the key fsc.logging.spec instead. Indeed, if you look at the code, a few lines before we set FSC_LOGGING_SPEC

Copy link
Contributor

Choose a reason for hiding this comment

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

my explanation is not completely correct.
This is the code that initializes the FSC logger:

func (p *provider) load() error {
	p.v = viper.New()
	err := p.initViper(p.v, CmdRoot)
	if err != nil {
		return err
	}

	err = p.v.ReadInConfig() // Find and read the config file
	if err != nil {          // Handle errors reading the config file
		// The version of Viper we use claims the config type isn't supported when in fact the file hasn't been found
		// Display a more helpful message to avoid confusing the user.
		if strings.Contains(fmt.Sprint(err), "Unsupported Config Type") {
			return errors.Errorf("Could not find config file. "+
				"Please make sure that FSCNODE_CFG_PATH is set to a path "+
				"which contains %s.yaml", CmdRoot)
		} else {
			return errors.WithMessagef(err, "error when reading %s config file", CmdRoot)
		}
	}

	// read in the legacy logging level settings and, if set,
	// notify users of the FSCNODE_LOGGING_SPEC env variable
	var loggingLevel string
	if p.v.GetString("logging_level") != "" {
		loggingLevel = p.v.GetString("logging_level")
	} else {
		loggingLevel = p.v.GetString("logging.level")
	}
	if loggingLevel != "" {
		logger.Warning("CORE_LOGGING_LEVEL is no longer supported, please use the FSCNODE_LOGGING_SPEC environment variable")
	}

	loggingSpec := os.Getenv("FSCNODE_LOGGING_SPEC")
	loggingFormat := os.Getenv("FSCNODE_LOGGING_FORMAT")

	if len(loggingSpec) == 0 {
		loggingSpec = p.v.GetString("logging.spec")
	}
	if len(loggingFormat) == 0 {
		loggingFormat = p.v.GetString("logging.format")
	}

	flogging.Init(flogging.Config{
		Format:  loggingFormat,
		Writer:  logOutput,
		LogSpec: loggingSpec,
	})

	return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, you can take the chance and cleanup this code. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I've removed the legacy logging level flag and pass now FSCNODE_LOGGING_SPEC via nwo.

Currently, NWO uses the deprecated --logging-level param when invoking
the peer cli. This commit replaces this param with FABRIC_LOGGING_SPEC
env variable.

Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

LGTM

@adecaro adecaro merged commit 7d4209f into hyperledger-labs:main May 23, 2024
18 checks passed
@mbrandenburger mbrandenburger deleted the cli-logging branch May 23, 2024 11:55
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.

3 participants