You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/11 13:55:36 UTC
[GitHub] [pulsar] flowchartsman opened a new pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
flowchartsman opened a new pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013
Fixes #9772
### Motivation
#9772
### Modifications
Add context `RecordMetric(metricName string, metricValue float64)`
### Verifying this change
The current testing framework doesn't exercise pfunc metrics at all. If there are integration tests elsewhere that can verify this, let me know, but I have tested it in my environment and it works as intended.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API: **yes** (pulsar function SDK only)
- It brings the context object in-line with the other two frameworks, however it follows the same signature.
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [ ] doc-required
- [ ] no-need-doc
- [X] doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-1042535348
If there's something I need to fix, could we get another test run, please? I cannot access the logs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#discussion_r707149794
##########
File path: site2/docs/functions-develop.md
##########
@@ -1006,7 +1006,22 @@ class MetricRecorderFunction(Function):
context.record_metric('elevens-count', 1)
```
<!--Go-->
-Currently, the feature is not available in Go.
+The Go SDK [`Context`](#context) object enables you to record metrics on a per-key basis. For example, you can set a metric for the `process-count` key and a different metric for the `elevens-count` key every time the function processes a message:
Review comment:
@freeznet could you please help review this from the technical perspective? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nlu90 commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
nlu90 commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-1024488934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-973047607
Cool! Is anything further from me required to get this merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman edited a comment on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman edited a comment on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-920932164
> And please add some tests, thanks.
Will `TestMetricsServer()` tests be enough? As near as I can tell, this is the only place where Go function metrics are tested, unless there are some integration tests in Java somewhere that I missed. Otherwise, it will involve a lot of work to add a testing framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-921256647
Comment requested on latest batch of changes. Tests will drop when I figure out where to put them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-1043363328
Style failure looks like a network issue not (yet) a style one:
```
go: golang.org/x/tools@v0.0.0-20180917221912-90fa682c2a6e: git fetch -f origin refs/heads/*:refs/heads/* refs/tags/*:refs/tags/* in /home/runner/go/pkg/mod/cache/vcs/b44680b3c3708a854d4c89f55aedda0b223beb8d9e30fba969cefb5bd9c1e843: exit status 128:
error: 9712 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output
```
Especially since go1.13 style check passed. Could we get a re-run?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on a change in pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on a change in pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#discussion_r707271695
##########
File path: site2/docs/functions-develop.md
##########
@@ -1006,7 +1006,22 @@ class MetricRecorderFunction(Function):
context.record_metric('elevens-count', 1)
```
<!--Go-->
-Currently, the feature is not available in Go.
+The Go SDK [`Context`](#context) object enables you to record metrics on a per-key basis. For example, you can set a metric for the `process-count` key and a different metric for the `elevens-count` key every time the function processes a message:
Review comment:
Worth noting that I more or less copied this from the Java one, since the method signature is the same and they use the same technique of overloading a single metric with labels.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on a change in pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
freeznet commented on a change in pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#discussion_r709726464
##########
File path: pulsar-function-go/pf/instance.go
##########
@@ -75,9 +77,20 @@ func newGoInstance() *goInstance {
return producer
}
+ metricsLabels := goInstance.getMetricsLabels()
+
+ var userMetrics sync.Map
+ goInstance.context.recordMetric = func(metricName string, metricValue float64) {
+ v, ok := userMetrics.Load(metricName)
+ if !ok {
+ v, _ = userMetrics.LoadOrStore(metricName, userMetricSummary.WithLabelValues(append(metricsLabels, metricName)...))
+ }
+ v.(prometheus.Observer).Observe(metricValue)
+ }
+
Review comment:
Can we move this part to `context.go`?
##########
File path: pulsar-function-go/pf/stats.go
##########
@@ -122,6 +126,11 @@ var (
prometheus.GaugeOpts{
Name: PulsarFunctionMetricsPrefix + "system_exception",
Help: "Exception from system code."}, exceptionMetricsLabelNames)
+
+ userMetricSummary = prometheus.NewSummaryVec(
+ prometheus.SummaryOpts{
+ Name: PulsarFunctionMetricsPrefix + UserMetric,
+ Help: "Pulsar Function user defined metric"}, userMetricLabelNames)
Review comment:
please make sure the go's metrics are same as the java's
https://github.com/apache/pulsar/blob/3231caa894a24fa9048adf4628316d280d02c679/pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/ContextImpl.java#L204-L214
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-940474630
Good to merge then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] addisonj commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
addisonj commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-1043366899
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nlu90 commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
nlu90 commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-973155128
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-950525328
Would love to get this merged this so I can remove some replace directives :) Am I missing anything?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-940606926
@nlu90 @wolfstudy PTAL when you have time, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-939394508
@merlimat @wolfstudy @freeznet Think we should be good now, if anyone wants to help me put it to bed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-920932164
> And please add some tests, thanks.
Will `TestMetricsServer()` tests be enough? As near as I can tell, this is the only place where Go function metrics are tested, unless there are some integration tests in Java somewhere that I missed. Otherwise, it will involve a lot of work to add a testing framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] flowchartsman commented on pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013#issuecomment-1024372112
Is anything blocking this from being merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet merged pull request #12013: [Issue 9772][Go Functions]Allow User Metrics
Posted by GitBox <gi...@apache.org>.
freeznet merged pull request #12013:
URL: https://github.com/apache/pulsar/pull/12013
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org