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

implemnt Prometheus sink #663

Closed
wants to merge 7 commits into from
Closed

implemnt Prometheus sink #663

wants to merge 7 commits into from

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Jul 31, 2024

this patch try to add native prometheus support instead of a statsd_exporter sidecar container.

@mattklein123 can you take a first look, I will finish the test if this's acceptable.

@mattklein123
Copy link
Member

Sure sounds good. Please add docs also.

Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
@mattklein123 mattklein123 self-assigned this Aug 2, 2024
@SpecialYang
Copy link

With the following rate limit policy, the process will crash.

apiVersion: v1
kind: ConfigMap
metadata:
  name: ratelimit-config
data:
  config.yaml: |
    domain: ratelimit
    descriptors:
      - key: PATH
        value: "/version"
        rate_limit:
          unit: minute
          requests_per_unit: 10
        descriptors:
        - key: test
          value: true
          rate_limit:
            unit: minute
            requests_per_unit: 2
panic: inconsistent label cardinality: expected 3 label values but got 2 in []string{"ratelimit", "PATH_/version"}

goroutine 21 [running]:
github.com/prometheus/client_golang/prometheus.(*CounterVec).WithLabelValues(...)
	/go/pkg/mod/github.com/prometheus/[email protected]/prometheus/counter.go:284
github.com/envoyproxy/ratelimit/src/prometheusstats.(*prometheusSink).FlushCounter(0xc00013c4b0, {0xc0004c20c0?, 0xc0004ce690?}, 0x1b)
	/ratelimit/src/prometheusstats/prometheus_sink.go:122 +0x4ec
github.com/lyft/gostats.(*statStore).Flush.func1({0xd434e0?, 0xc0004dd580?}, {0xe36580?, 0xc000392d00?})
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:386 +0x76
sync.(*Map).Range(0xc0000d36e0?, 0xc0004a7f08)
	/usr/local/go/src/sync/map.go:477 +0x1f8
github.com/lyft/gostats.(*statStore).Flush(0xc0004d6320)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:383 +0xbf
github.com/lyft/gostats.(*statStore).StartContext(0xc0004d6320, {0x1083980, 0x1888980}, 0xc00013d770)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:367 +0x35
github.com/lyft/gostats.(*statStore).Start(0x0?, 0x0?)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:373 +0x25
created by github.com/envoyproxy/ratelimit/src/service_cmd/runner.NewRunner in goroutine 1
	/ratelimit/src/service_cmd/runner/runner.go:77 +0x74b

@zirain
Copy link
Contributor Author

zirain commented Aug 6, 2024

With the following rate limit policy, the process will crash.

apiVersion: v1
kind: ConfigMap
metadata:
  name: ratelimit-config
data:
  config.yaml: |
    domain: ratelimit
    descriptors:
      - key: PATH
        value: "/version"
        rate_limit:
          unit: minute
          requests_per_unit: 10
        descriptors:
        - key: test
          value: true
          rate_limit:
            unit: minute
            requests_per_unit: 2
panic: inconsistent label cardinality: expected 3 label values but got 2 in []string{"ratelimit", "PATH_/version"}

goroutine 21 [running]:
github.com/prometheus/client_golang/prometheus.(*CounterVec).WithLabelValues(...)
	/go/pkg/mod/github.com/prometheus/[email protected]/prometheus/counter.go:284
github.com/envoyproxy/ratelimit/src/prometheusstats.(*prometheusSink).FlushCounter(0xc00013c4b0, {0xc0004c20c0?, 0xc0004ce690?}, 0x1b)
	/ratelimit/src/prometheusstats/prometheus_sink.go:122 +0x4ec
github.com/lyft/gostats.(*statStore).Flush.func1({0xd434e0?, 0xc0004dd580?}, {0xe36580?, 0xc000392d00?})
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:386 +0x76
sync.(*Map).Range(0xc0000d36e0?, 0xc0004a7f08)
	/usr/local/go/src/sync/map.go:477 +0x1f8
github.com/lyft/gostats.(*statStore).Flush(0xc0004d6320)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:383 +0xbf
github.com/lyft/gostats.(*statStore).StartContext(0xc0004d6320, {0x1083980, 0x1888980}, 0xc00013d770)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:367 +0x35
github.com/lyft/gostats.(*statStore).Start(0x0?, 0x0?)
	/go/pkg/mod/github.com/lyft/[email protected]/stats.go:373 +0x25
created by github.com/envoyproxy/ratelimit/src/service_cmd/runner.NewRunner in goroutine 1
	/ratelimit/src/service_cmd/runner/runner.go:77 +0x74b

can you share more logs?

Signed-off-by: zirain <[email protected]>
@zirain
Copy link
Contributor Author

zirain commented Aug 12, 2024

@SpecialYang can you share the reproduce steps?

@zirain
Copy link
Contributor Author

zirain commented Aug 13, 2024

There are a lot of problems with this implementation, I'd like close this one first, and proposal a new one later.

@zirain zirain closed this Aug 13, 2024
@zirain zirain mentioned this pull request Aug 14, 2024
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.

3 participants