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

Update tests and doc comments to use SERVER_URL #858

Merged
merged 1 commit into from Dec 10, 2020

Conversation

axw
Copy link
Member

@axw axw commented Dec 10, 2020

The Go Agent will continue to support SERVER_URLS,
but we will de-epmphasise its use in favour of using
load balancer proxies with agents generally. We never
actually documented SERVER_URLS in the user docs,
so nothing to do there.

Closes #850

The Go Agent will continue to support SERVER_URLS,
but we will de-epmphasise its use in favour of using
load balancer proxies with agents generally. We never
actually documented SERVER_URLS in the user docs,
so nothing to do there.
@axw axw requested a review from a team December 10, 2020 02:24
@apmmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #858 opened

  • Start Time: 2020-12-10T02:24:30.127+0000

  • Duration: 25 min 27 sec

Test stats 🧪

Test Results
Failed 0
Passed 7650
Skipped 188
Total 7838

// use the default URL "http://localhost:8200".
// - ELASTIC_APM_SERVER_URL: the APM Server URL used for sending
// requests. If no URL is specified, then the transport will use the
// default URL "http://localhost:8200".
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor thing, but since ELASTIC_APM_SERVER_URLS is deprecated, should we also change the order of how the ENV vars are parsed, so that ELASTIC_APM_SERVER_URL is always used if provided.

// initServerURLs parses ELASTIC_APM_SERVER_URLS if specified,
// otherwise parses ELASTIC_APM_SERVER_URL if specified. If
// neither are specified, then the default localhost URL is
// returned.
func initServerURLs() ([]*url.URL, error) {
key := envServerURLs
value := os.Getenv(key)
if value == "" {
key = envServerURL
value = os.Getenv(key)
}
.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did think about doing that, and decided at the time to make it a purely non-functional change. I don't think it's likely to matter either way, but I'd like to avoid a breaking change unless necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume since you approved that you're good either way, so I'll merge. If you disagree with my decision, let me know and we can come back to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I am fine either way - thanks for the explanation.

@axw axw merged commit 879c34a into elastic:master Dec 10, 2020
@axw axw deleted the server-url-singular branch December 10, 2020 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 359] Consolidate server_url/server_urls option
3 participants