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

GODRIVER-2904 Add environment log. #1373

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

qingyang-hu
Copy link
Collaborator

GODRIVER-2904

Summary

Log informational message client-side based on detected environment (DocumentDB or CosmosDB)

Background & Motivation

Log informational messages by examining host names added to a topology.
SRV connection strings are not tested, since initial lookup responses cannot be easily mocked.

@qingyang-hu qingyang-hu marked this pull request as ready for review September 5, 2023 18:40
@qingyang-hu qingyang-hu requested a review from a team as a code owner September 5, 2023 18:40
@qingyang-hu qingyang-hu requested review from prestonvasquez and blink1073 and removed request for a team September 5, 2023 18:40
blink1073
blink1073 previously approved these changes Sep 6, 2023
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!


var (
cosmosDBLog = `You appear to be connected to a CosmosDB cluster. For more information regarding feature compatibility and support please visit https://www.mongodb.com/supportability/cosmosdb`
doumentDBLog = `You appear to be connected to a DoumentDB cluster. For more information regarding feature compatibility and support please visit https://www.mongodb.com/supportability/documentdb`
Copy link
Contributor

Choose a reason for hiding this comment

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

DoumentDB -> DocumentDB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you!


var (
cosmosDBLog = `You appear to be connected to a CosmosDB cluster. For more information regarding feature compatibility and support please visit https://www.mongodb.com/supportability/cosmosdb`
documentDBLog = `You appear to be connected to a DocumentDB cluster. For more information regarding feature compatibility and support please visit https://www.mongodb.com/supportability/documentdb`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest making cosmosDBLog and documentDBLog constants. See comments on restructuring.

}
}

func (l *hostLogger) log(host string) {
Copy link
Collaborator

@prestonvasquez prestonvasquez Sep 6, 2023

Choose a reason for hiding this comment

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

I don't think we should have a struct for logging. IIUC the only reason we do this is to maintain state so as not to repeated logs for the same host. However, it might be best to maintain this state in the code that is calling the logger, to minimize any leaking:

			hostSet := map[string]bool{}
			for _, host := range parsedHosts {
				if h, _, err := net.SplitHostPort(host); err == nil {
					host = h
				}

				if !hostSet[host] {
					logTopoloygThirdPartyUsage(t, host)
				}

				hostSet[host] = true
			}

}
}

func (l *hostLogger) log(host string) {
Copy link
Collaborator

@prestonvasquez prestonvasquez Sep 6, 2023

Choose a reason for hiding this comment

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

Whether we decided to use a struct or not, we should re-organize the code to match the existing "logTopology*" pattern:

func logTopologyThirdPartyUsage(topo *Topology, host string) {
	thirdPartyMessages := map[string]string{
		".cosmos.azure.com":            cosmosDBLog,
		".docdb.amazonaws.com":         documentDBLog,
		".docdb-elastic.amazonaws.com": documentDBLog,
	}

	for tphost, msg := range thirdPartyMessages {
		if !strings.HasSuffix(host, tphost) {
			continue
		}

		// Serialize the topology message to be consistent with other topology
		// messages.
		topo.cfg.logger.Print(logger.LevelInfo,
			logger.ComponentTopology,
			msg,
			logger.SerializeTopology(logger.Topology{ID: topo.id, Message: msg})...)
	}
}

} else {
logger := newHostLogger(t.cfg.logger)
for _, host := range parsedHosts {
logger.log(host)
Copy link
Collaborator

@prestonvasquez prestonvasquez Sep 6, 2023

Choose a reason for hiding this comment

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

Before we log, we first need to check if it is appropriate to log. We should update mustLogTopologyMessage to the following:

func mustLogTopologyMessage(topo *Topology, level logger.Level) bool {
	return topo.cfg.logger != nil && topo.cfg.logger.LevelComponentEnabled(
		level, logger.ComponentTopology)
}

and then wrap this block:

		if mustLogTopologyMessage(t, logger.LevelInfo) {
			hostSet := map[string]bool{}
			for _, host := range parsedHosts {
				if h, _, err := net.SplitHostPort(host); err == nil {
					host = h
				}

				if !hostSet[host] {
					logTopologyThirdPartyUsage(t, host)
				}

				hostSet[host] = true
			}
		}

@qingyang-hu
Copy link
Collaborator Author

The check is done on the SRV's CNAME prior to SRV lookup as per the comment.

return err
}
parsedHosts := strings.Split(uri.Host, ",")
if mustLogTopologyMessage(t, logger.LevelInfo) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: the only purpose of this block is to log third party connection information. It could be cleaner to create a separate function called logTopologyThirdPartyUsage to hold all of this logic:

if mustLogTopologyMessage(t, logger.LevelInfo) {
    logTopologyThirdPartyUsage(...)
}

}
if logged, ok := hostSet[env]; ok && logged {
break
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: this else is not necessary, the logic can be written like this:

				if !strings.HasSuffix(host, suffix) {
					continue
				}
				
				if logged, ok := hostSet[env]; ok && logged {
					break
				} 
				
				hostSet[env] = true
				logTopologyMessage(t, logger.LevelInfo, thirdPartyMessages[env])

)

var (
thirdPartySuffixes = map[string]hostEnv{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: If we create a separate function (logTopologyThirdPartyUsage), then there is no need to create package-level variables, we can nest these declarations within the logging function to give them context.

{
name: "normal",
uri: "mongodb://a.mongo.cosmos.azure.com:19555/",
msgs: []string{cosmosDBMsg},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should directly reference the cosmosDBMsg in the test, and not use a non-test constant. There are a few reasons for this:

  1. Test code won't change when production code is refactored. That is, if we use a constant here then refactoring the constant in the production code would break this test.
  2. These tests might pass if the code is wrong. Any changes to the constants will be considered valid, for instance if we accidentally append a character to the constant.

This applies to the other test cases as well.

err = topo.Connect()
require.Nil(t, err, "Connect error: %v", err)

require.ElementsMatch(t, tc.msgs, sink.msgs, "expected messages to be %v, got %v", tc.msgs, sink.msgs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use an assertion here? require will stop testing if it fails. This applies to the other subtests as well.


// Note: SRV connection strings are intentionally untested, since initial
// lookup responses cannot be easily mocked.
func TestTopologyConstructionLogging(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can test can be parallelized? From my testing it appears it can be.

// lookup responses cannot be easily mocked.
func TestTopologyConstructionLogging(t *testing.T) {
sink := &mockLogSink{}
loggerOptions := options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: if these are re-defined for each subtest, then we can parallelize the subtests.

@qingyang-hu qingyang-hu added this pull request to the merge queue Sep 12, 2023
Merged via the queue into mongodb:master with commit 101d1e7 Sep 12, 2023
1 check failed
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.

4 participants