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

[faucet] Add custom metrics #1143

Merged
merged 1 commit into from
Oct 1, 2019
Merged

[faucet] Add custom metrics #1143

merged 1 commit into from
Oct 1, 2019

Conversation

mcortesi
Copy link
Contributor

Description

Log events based on execution result

Tested

Deploy and faucet. Then check logs

Other changes

Yarn lock updated a couple of things, don't know why. I've only added google cloudl ogging library

Related issues

Log events based on execution result
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
- Coverage    66.7%   66.59%   -0.11%     
==========================================
  Files         259      257       -2     
  Lines        7454     7394      -60     
  Branches      432      494      +62     
==========================================
- Hits         4972     4924      -48     
+ Misses       2386     2373      -13     
- Partials       96       97       +1
Flag Coverage Δ
#mobile 66.59% <ø> (-0.11%) ⬇️
Impacted Files Coverage Δ
packages/mobile/src/localCurrency/selectors.ts 57.14% <0%> (-12.43%) ⬇️
packages/mobile/src/localCurrency/saga.ts 65.38% <0%> (-5.59%) ⬇️
...kages/mobile/src/transactions/TransferFeedItem.tsx 80.89% <0%> (-0.22%) ⬇️
packages/mobile/src/home/CeloDollarsOverview.tsx 100% <0%> (ø) ⬆️
packages/mobile/src/navigator/Navigator.tsx 0% <0%> (ø) ⬆️
packages/mobile/src/utils/formatting.ts 87.5% <0%> (ø) ⬆️
packages/mobile/src/send/TransferReviewCard.tsx 96.87% <0%> (ø) ⬆️
packages/mobile/src/navigator/Screens.tsx 100% <0%> (ø) ⬆️
packages/mobile/src/localCurrency/actions.ts 100% <0%> (ø) ⬆️
...le/src/paymentRequest/PaymentRequestReviewCard.tsx 96.29% <0%> (ø) ⬆️
... and 10 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 ae13eb8...9bd89d5. Read the comment docs.

Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

Just a small suggestion about using STDOUT instead of directly hitting GCP

@@ -0,0 +1,52 @@
import { Logging } from '@google-cloud/logging'
Copy link
Contributor

Choose a reason for hiding this comment

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

So of course we do not have any standardization so to speak, but I generally would prefer for us not to directly log to GCP. Instead, GCP already aggregates the logs for us from STDOUT/STDERR and deals with doing so in a performant manner, batch, etc.

It also avoids us having to configure the client below, give the appropriate labels like project/region, and many more useful information such as execution id, severity etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, I agree. But this is different.
The reason I'm using this library is to create my own LogEntry for stackdriver.
See that I'm using:

  • A different resource (fantasy function name on the metadata)
  • structured log entry, not a string

See the screenshot of an actual log:
image

This allow to easily create new metric with metric labels using structured fields.

@mcortesi mcortesi added the automerge Have PR merge automatically when checks pass label Oct 1, 2019
@mcortesi mcortesi merged commit 053e2ae into master Oct 1, 2019
aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (31 commits)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  [cli] Solution for build error contractkit on Linux 19.04 distro (#960)
  [Wallet] Merge back changes made for mx pilot (#1113)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (35 commits)
  [Wallet] Network fee in transaction feed (#1145)
  New About Page Cover (#905)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  ...

# Conflicts:
#	packages/web/src/about/About.tsx
#	packages/web/src/about/images/index.ts
#	packages/web/static/locales/en/about.json
@mcortesi mcortesi deleted the mc/faucet/metrics branch December 4, 2019 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants