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

Move TelemetryURL to SRV Record #374

Merged
merged 30 commits into from Oct 16, 2019
Merged

Move TelemetryURL to SRV Record #374

merged 30 commits into from Oct 16, 2019

Conversation

winder
Copy link
Contributor

@winder winder commented Oct 7, 2019

Summary

Lookup telemetry URL via SRV records.

Test Plan

Created telemetry.devnet.algodev.network SRV record and have been using that for testing.

@winder winder changed the title Move Move TelemetryURL to SRV Record Oct 7, 2019
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Change the function ReadFromBootstrap into GetSRVRecords, and have it receive the following parameters:
bootstrapID, fallbackDNSResoverAddress, serviceName and log.

Instead of creating a sub-domain with a SRV record, you want to create a SRV record with a different service name on the same domain.

cmd/algod/main.go Outdated Show resolved Hide resolved
cmd/algod/main.go Show resolved Hide resolved
logging/telemetryhook.go Outdated Show resolved Hide resolved
logging/telemetryhook.go Outdated Show resolved Hide resolved
logging/telemetryhook.go Outdated Show resolved Hide resolved
@winder
Copy link
Contributor Author

winder commented Oct 10, 2019

@tsachiherman I think I addressed all of the feedback, added the following:

  • signal to async telemetry hook event loop on telemetry uri change instead of ticker.
  • service with a shutdown signal to manage the telemetry update loop, used by algod/algoh.
  • shutdown signal added to the heartbeat event loop

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Few more comments. see below.

tools/network/bootstrap.go Outdated Show resolved Hide resolved
tools/network/telemetryURIUpdateService.go Outdated Show resolved Hide resolved
tools/network/telemetryURIUpdateService.go Outdated Show resolved Hide resolved
tsachiherman
tsachiherman previously approved these changes Oct 11, 2019
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good. thanks for the changes.

@@ -165,6 +169,10 @@ func createElasticHook(cfg TelemetryConfig) (hook logrus.Hook, err error) {
}
hostName := cfg.getHostName()
hook, err = elogrus.NewElasticHook(client, hostName, cfg.MinLogLevel, cfg.ChainID)

if err == nil && hook == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's pretty untypical for go; usually the object and the error are mutually exclusive.
If this is the case, don't return the hook if you know that its nil. return nil instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this doesn't actually happen and I was confused initially because nil, nil is returned when initialized with an empty telemetry string. I'll remove this and make sure that is really the case.

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

LGTM

@winder winder merged commit 01dfd6e into algorand:master Oct 16, 2019
@winder winder deleted the will/srv branch October 16, 2019 19:35
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.

None yet

2 participants