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

Emit metric for redis and bigtable client #210

Merged
merged 4 commits into from
Feb 3, 2022
Merged

Conversation

tiopramayudi
Copy link
Collaborator

What this PR does / why we need it:

Emit redis & bigtable client metric for better observability of feast direct serving
Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:


Checklist

  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduce API changes

@tiopramayudi tiopramayudi changed the title Storage metric Emit metric for redis and bigtable client Jan 21, 2022
Help: "Redis connection stats",
}, []string{"stat"})

hitConnStats = "hits"
Copy link
Member

@pradithya pradithya Jan 21, 2022

Choose a reason for hiding this comment

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

Use constant instead of var

}

func (h *redisHook) AfterProcess(ctx context.Context, cmd redis.Cmder) error {
startTime, ok := ctx.Value(requestStartKey{}).(time.Time)
Copy link
Member

Choose a reason for hiding this comment

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

How will it be able to calculate the latency? It doesn't seem to be the case here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before running the redis command, it will create new context that add start time of the command, this part of BeforeProcess

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it!

Copy link
Member

@pradithya pradithya left a comment

Choose a reason for hiding this comment

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

LGTM

@tiopramayudi tiopramayudi merged commit fa67164 into main Feb 3, 2022
@tiopramayudi tiopramayudi deleted the storage-metric branch February 3, 2022 05:20
pradithya pushed a commit that referenced this pull request Oct 3, 2022
* Add metric for redis and bigtable direct serving

* Check whether pool stats for redis nil or not

* Make status string as const instead of var

* Remove bigtable grpc metric

Co-authored-by: Tio Pramayudi <[email protected]>
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.

2 participants