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.

Sign up for GitHub

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

Jump to bottom

Document and fix recorded metric forwarding #266

Merged
merged 2 commits into from
Apr 29, 2022
Merged

Document and fix recorded metric forwarding #266

merged 2 commits into from
Apr 29, 2022

Conversation

n-oden
Copy link
Contributor

@n-oden n-oden commented Jan 15, 2021 •
edited
Loading

This should address most of #104

  • fix target cache lookup for recorded metrics
  • update documentation with examples

Also: allow overriding docker FROM target with DOCKER_IMAGE_BASE env var
(helpful for testing and runtime scenarios where /bin/sh is needed)

@google-cla google-cla bot added the cla: yes label Jan 15, 2021
@memory memory mentioned this pull request Jan 15, 2021
Closed
@n-oden n-oden mentioned this pull request Jan 15, 2021
Closed
@n-oden
Copy link
Contributor Author

n-oden commented Jan 16, 2021

The build failure here seems to be unrelated to the PR:

Makefile:98: *** COVERALLS_TOKEN is undefined, follow https://docs.coveralls.io/go to create one and go to https://coveralls.io to retrieve existing ones.  Stop.
The command "make goveralls" exited with 2.

igorpeshansky
igorpeshansky requested changes Mar 16, 2021
View reviewed changes
targets/cache.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
opencensus/doc.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@igorpeshansky
Copy link
Member

The COVERALLS_TOKEN error was fixed in #274. You probably need to rebase anyway to fix the conflicts.

@n-oden
Copy link
Contributor Author

n-oden commented Apr 28, 2021

Hey @igorpeshansky thanks for the feedback! Sorry I haven't been able to get to this yet but it's on my last for May.

@n-oden n-oden force-pushed the recorded-metric-fix-and-docs branch from d0f7588 to 64a5e2a Compare December 27, 2021 21:17
@n-oden n-oden requested a review from igorpeshansky December 27, 2021 21:17
@n-oden
Copy link
Contributor Author

n-oden commented Dec 27, 2021

So, uh, obviously this is a bit later than May. But I've updated the PR with all of your comments hopefully addressed, and it's running happily in our testing environment!

@n-oden n-oden force-pushed the recorded-metric-fix-and-docs branch from 64a5e2a to e07e745 Compare December 28, 2021 14:58
@n-oden
Copy link
Contributor Author

n-oden commented Jan 3, 2022

@igorpeshansky happy new year! Sorry for letting this slide for so long since your review; if you get a moment I think it's more or less good to go now.

@n-oden
Copy link
Contributor Author

n-oden commented Feb 8, 2022

@davidbtucker @igorpeshansky gentle ping :)

@n-oden
Copy link
Contributor Author

n-oden commented Mar 11, 2022

@davidbtucker @igorpeshansky gentle re-ping :)

Sorry to be a pest about this; if it sweetens the pot at all, what I'm hoping to get out of this is a nice long public blog post about how to use the sidecar to pull metrics out of GKE clusters using the Linkerd service mesh (rather than Istio) and get them into Google Service Monitoring as SLOs.

igorpeshansky
igorpeshansky requested changes Apr 29, 2022
View reviewed changes
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

Sorry for letting this fall through the cracks.

README.md Outdated Show resolved Hide resolved
n-oden added 2 commits April 29, 2022 08:32
@n-oden
Document and fix recorded metric forwarding
e9317fc 
This should address most of Stackdriver#104

- fix target cache lookup for recorded metrics
- update documentation with examples

Also: allow overriding docker FROM target with DOCKER_IMAGE_BASE env var
(helpful for testing and runtime scenarios where /bin/sh is needed)
@n-oden
punctuation
03c8152
@n-oden n-oden force-pushed the recorded-metric-fix-and-docs branch from e07e745 to 03c8152 Compare April 29, 2022 12:34
@n-oden n-oden requested a review from igorpeshansky April 29, 2022 12:34
@n-oden
Copy link
Contributor Author

n-oden commented Apr 29, 2022 •
edited
Loading

@igorpeshansky no worries -- multiple months of the lag were on my end so I'm in no position to cast stones. Looking forward to getting this merged!

Once it is, can I agitate for an 0.11 release?

igorpeshansky
igorpeshansky approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@igorpeshansky igorpeshansky merged commit a8c2f78 into Stackdriver:master Apr 29, 2022
@n-oden n-oden deleted the recorded-metric-fix-and-docs branch April 29, 2022 15:50
@igorpeshansky
Copy link
Member

Once it is, can I agitate for an 0.11 release?

This was a bug fix, not a new feature, so we didn't bump the minor version. 0.10.1 has just been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorpeshansky igorpeshansky igorpeshansky approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@n-oden @igorpeshansky