Skip to content

Document and fix recorded metric forwarding#266

Merged
igorpeshansky merged 2 commits into
Stackdriver:masterfrom
odenio:recorded-metric-fix-and-docs
Apr 29, 2022
Merged

Document and fix recorded metric forwarding#266
igorpeshansky merged 2 commits into
Stackdriver:masterfrom
odenio:recorded-metric-fix-and-docs

Conversation

@n-oden

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

Copy link
Copy Markdown
Contributor

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

n-oden commented Jan 16, 2021

Copy link
Copy Markdown
Contributor Author

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 suggested changes Mar 16, 2021
View reviewed changes
Comment thread targets/cache.go Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread opencensus/doc.go Outdated
Comment thread README.md
@igorpeshansky

igorpeshansky commented Mar 16, 2021

Copy link
Copy Markdown
Contributor

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

@n-oden

n-oden commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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 requested a review from igorpeshansky December 27, 2021 21:17
@n-oden

n-oden commented Dec 27, 2021

Copy link
Copy Markdown
Contributor Author

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 commented Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

@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

n-oden commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

@davidbtucker @igorpeshansky gentle ping :)

@n-oden

n-oden commented Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

@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 suggested changes Apr 29, 2022
View reviewed changes

@igorpeshansky igorpeshansky left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread README.md Outdated
n-oden added 2 commits April 29, 2022 08:32
@n-oden
Document and fix recorded metric forwarding
e9317fc 
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)
@n-oden
punctuation
03c8152
@n-oden n-oden requested a review from igorpeshansky April 29, 2022 12:34
@n-oden

n-oden commented Apr 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

@igorpeshansky igorpeshansky left a comment

Copy link
Copy Markdown
Contributor

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

igorpeshansky commented May 4, 2022

Copy link
Copy Markdown
Contributor

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

1 more reviewer

@igorpeshansky igorpeshansky igorpeshansky approved these changes

Reviewers whose approvals may not affect merge requirements

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