Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rename metric name for request latency in feast serving #488

Conversation

davidheryanto
Copy link
Collaborator

What this PR does / why we need it:
So that it is consistent with the actual unit of timing being measured.
And recommended metric names in Prometheus https://prometheus.io/docs/practices/naming/#metric-names
Which issue(s) this PR fixes:

NONE

Does this PR introduce a user-facing change?:

NONE

So that it is consistent with the actual unit of timing being measured
And recommended metric names in Prometheus https://prometheus.io/docs/practices/naming/#metric-names
@davidheryanto
Copy link
Collaborator Author

/retest

1 similar comment
@davidheryanto
Copy link
Collaborator Author

/retest

@davidheryanto
Copy link
Collaborator Author

/hold
Checking why test fails

@davidheryanto
Copy link
Collaborator Author

Test fails because several hours ago fastavro is updated to 0.22.10 and this line wil fail

to_avro(df=features_1_df[["event_timestamp", "entity_id"]], file_path_or_buffer=file_name)

with this error

TypeError: Timestamp subtraction must have the same timezones or no timezones

2 potential fixes:

  • Fix the fastavro version to 0.22.9
  • Do not specify timezone in the end-to-end test sample data
- time_offset = datetime.utcnow().replace(tzinfo=pytz.utc)
+ time_offset = datetime.utcnow().replace(tzinfo=None)

@davidheryanto
Copy link
Collaborator Author

/retest

@davidheryanto davidheryanto force-pushed the fix-prometheus-metric-name-in-feast-serving branch from 74be918 to 3e5be55 Compare February 25, 2020 05:33
@woop
Copy link
Member

woop commented Feb 25, 2020

Can we merge this @davidheryanto ?

@davidheryanto
Copy link
Collaborator Author

/hold cancel
Yes the master already fix fastavro version to 0.22.9
I forgot to cancel the hold label

@woop
Copy link
Member

woop commented Feb 26, 2020

/lgtm
/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidheryanto, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit 5758d99 into feast-dev:master Feb 26, 2020
davidheryanto added a commit to davidheryanto/feast that referenced this pull request Feb 26, 2020
So that it is consistent with the actual unit of timing being measured
And recommended metric names in Prometheus https://prometheus.io/docs/practices/naming/#metric-names

(cherry picked from commit 5758d99)
zhilingc pushed a commit that referenced this pull request Feb 26, 2020
So that it is consistent with the actual unit of timing being measured
And recommended metric names in Prometheus https://prometheus.io/docs/practices/naming/#metric-names

(cherry picked from commit 5758d99)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants