Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Enable to receive credentials object in creating Stackdriver exporter#541

Merged
mayurkale22 merged 5 commits into
census-instrumentation:masterfrom
yamadayuki:yamadayuki/support-credentials-object
May 22, 2019
Merged

Enable to receive credentials object in creating Stackdriver exporter#541
mayurkale22 merged 5 commits into
census-instrumentation:masterfrom
yamadayuki:yamadayuki/support-credentials-object

Conversation

@yamadayuki

@yamadayuki yamadayuki commented May 21, 2019

Copy link
Copy Markdown
Contributor

To fix #537

This PR enables us to set the Stackdriver's credentials from the environment variables instead of ADC. c.f. https://cloud.google.com/docs/authentication/production
It's ok to use GOOGLE_APPLICATION_CREDENTIALS as before or use the credentials option passing to the StackdriverTraceExporter / StackdriverStatsExporter constructor.

import { StackdriverTraceExporter } from "@opencensus/exporter-stackdriver";

// load the environment variable with keys encoded with Base64
const jsonContent = Buffer.from(process.env.CREDENTIALS_BASE64, "base64").toString();
const credentials = JSON.parse(jsonContent);

const exporter = StackdriverTraceExporter({
  projectId: "project-id",
  credentails: credentials,
});

@codecov-io

codecov-io commented May 21, 2019

Copy link
Copy Markdown

Codecov Report

Merging #541 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   95.26%   95.09%   -0.17%     
==========================================
  Files         147      147              
  Lines        9969     9686     -283     
  Branches      543      530      -13     
==========================================
- Hits         9497     9211     -286     
- Misses        472      475       +3
Impacted Files Coverage Δ
src/trace/propagation/noop-propagation.ts 37.5% <0%> (-12.5%) ⬇️
src/internal/util.ts 90% <0%> (-10%) ⬇️
src/common/validations.ts 88.23% <0%> (-7.22%) ⬇️
src/tags/validation.ts 92.85% <0%> (-7.15%) ⬇️
test/test-zipkin.ts 91.66% <0%> (-6.07%) ⬇️
src/stackdriver-monitoring.ts 77.02% <0%> (-3.53%) ⬇️
test/test-jaeger-format.ts 94.91% <0%> (-1.24%) ⬇️
src/tags/propagation/text-format.ts 94.11% <0%> (-0.89%) ⬇️
src/tags/propagation/variant-encoding.ts 95% <0%> (-0.84%) ⬇️
src/prometheus-stats.ts 92.77% <0%> (-0.18%) ⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f522ce2...6b4b717. Read the comment docs.

mayurkale22
mayurkale22 reviewed May 21, 2019
View reviewed changes
Comment thread packages/opencensus-exporter-stackdriver/src/types.ts Outdated
prefix?: string;
/**
* Create a JWT instance using the given credentials input containing
* client_email and private_key properties. Optional

@mayurkale22 mayurkale22 May 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you update this field comment with: "If this field is set, its contents will be used for authentication instead of your application default credentials."?

@yamadayuki yamadayuki May 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK! Thanks for your suggestion.

mayurkale22
mayurkale22 approved these changes May 21, 2019
View reviewed changes

@mayurkale22 mayurkale22 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! Overall LGTM, Please update the CHANGELOG.md with this feat.

@mayurkale22

mayurkale22 commented May 21, 2019

Copy link
Copy Markdown
Member

@yamadayuki As we don't have system tests, did you verified these changes against the actual stackdriver instance?

@yamadayuki

yamadayuki commented May 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@yamadayuki As we don't have system tests, did you verified these changes against the actual stackdriver instance?

Yes. I released the opencensus-exporter-stackdriver including this feature to npm with own scope @yamadayuki. And I verified these changes using @yamadayuki/exporter-stackdriver in own applications. I confirmed this package exported the traces without GOOGLE_APPLICATION_CREDENTIALS.

@mayurkale22

mayurkale22 commented May 21, 2019

Copy link
Copy Markdown
Member

Yes. I released the opencensus-exporter-stackdriver including this feature to npm with own scope.

Awesome! Thanks for the work again!

@mayurkale22 mayurkale22 merged commit f68aa53 into census-instrumentation:master May 22, 2019
@yamadayuki yamadayuki deleted the yamadayuki/support-credentials-object branch May 23, 2019 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@draffensperger draffensperger Awaiting requested review from draffensperger draffensperger is a code owner
@justindsmith justindsmith Awaiting requested review from justindsmith
1 more reviewer
@mayurkale22 mayurkale22 mayurkale22 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.

Exporter/Stackdriver: Provide other way to set credential instead of ADC

4 participants

@yamadayuki @codecov-io @mayurkale22 @googlebot