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

Upgrade datadog to 2.2.0 for UDS bug fixes #722

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

prudhvi
Copy link
Contributor

@prudhvi prudhvi commented Apr 24, 2019

Summary

Upgrading the datadog client dependency to latest so that contains some features and bug fixes for Unix domain socket support.
https://github.com/DataDog/datadog-go/blob/master/CHANGELOG.md
Not adding a new CHANGELOG.md entry as it already contains my previous bump for ongoing version.

Motivation

To use the UDS client support with latest improvements and bug fixes.

@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur, Unilog, Falconer pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@asf-stripe asf-stripe self-assigned this May 27, 2019
@asf-stripe
Copy link
Contributor

Ah, sorry for missing this PR for so long! The change looks good, but can you rebase onto master?

@asf-stripe asf-stripe assigned prudhvi and unassigned asf-stripe May 27, 2019
@prudhvi prudhvi force-pushed the prudhvi.upgrade_datadog branch from 78c8a7c to a613928 Compare May 28, 2019 21:47
@prudhvi
Copy link
Contributor Author

prudhvi commented May 28, 2019

@asf-stripe Thanks, rebased with latest master

@asf-stripe asf-stripe force-pushed the prudhvi.upgrade_datadog branch from 7f7be99 to a613928 Compare May 29, 2019 10:36
@asf-stripe
Copy link
Contributor

So we had a small misconfiguration in our CI setup which required me to push an empty commit (now rebased away again) so that tests actually get run - now it looks like tests are properly failing; can you check them out?

@@ -320,7 +361,7 @@ func (c *Client) sendMsg(msg string) error {
// send handles sampling and sends the message over UDP. It also adds global namespace prefixes and tags.
func (c *Client) send(name string, value interface{}, suffix []byte, tags []string, rate float64) error {
if c == nil {
return nil
return fmt.Errorf("Client is nil")
Copy link
Contributor

Choose a reason for hiding this comment

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

The test failure seems to be related to this change. We weren't expecting a nil statsd client to raise, but maybe we ought to? I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Made DataDog/datadog-go#90 to fix this. Once it's merged & pulled in, we'll have to adjust the test case to check for the error constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks should have run locally before opening. Also TIL, go test ./... to run all tests.
Do you want me to change the constraint in gopkg.toml to specific sha and fix tests or should we actually wait for new release version of datadog client.

Copy link
Contributor

@asf-stripe asf-stripe May 31, 2019

Choose a reason for hiding this comment

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

I think it should be fine to update to the git version, especially if the bugfixes you are pulling in are blocking you!

Note that the test will need an update also, to validate that ErrNoClient is returned in the case of an empty client.

@prudhvi prudhvi force-pushed the prudhvi.upgrade_datadog branch from 16d6a51 to c0f2e20 Compare July 6, 2019 00:51
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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