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

feat: A new port is added for prometheus collection, and the indicator init… #1826

Closed
wants to merge 10 commits into from

Conversation

limowang
Copy link
Collaborator

@limowang limowang commented Jan 3, 2024

A new port is added for prometheus collection, and the indicator initialization process is extracted to form a separate function.

…ialization process is extracted to form a separate function.
@limowang limowang changed the title A new port is added for prometheus collection, and the indicator init… feat:A new port is added for prometheus collection, and the indicator init… Jan 4, 2024
@GehaFearless GehaFearless changed the title feat:A new port is added for prometheus collection, and the indicator init… feat: A new port is added for prometheus collection, and the indicator init… Jan 4, 2024
@@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

module github.com/pegasus-kv/collector
module github.com/limowang/incubator-pegasus/collector
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module github.com/limowang/incubator-pegasus/collector
module github.com/apache/incubator-pegasus/collector

collector/main Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Don't add binaries to the codebase.

// "github.com/pegasus-kv/collector/avail"
// "github.com/pegasus-kv/collector/metrics"
// "github.com/pegasus-kv/collector/webui"
"github.com/limowang/incubator-pegasus/collector/avail"
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comments and use the apache repository.

@@ -81,7 +84,8 @@ func main() {
return
}

webui.StartWebServer()
metrics.InitMetrics()
//webui.StartWebServer()
Copy link
Member

Choose a reason for hiding this comment

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

Just remove it if it's not needed.

@@ -53,9 +56,22 @@ func StartWebServer() {
app.RegisterView(tmpl)

go func() {
err := app.Listen(":8080")
err := app.Listen(":8081")
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to read from config file or command line rather than hard code.

The port should be user-defined to avoid port conflicts in user environments.

@@ -167,17 +149,15 @@ func initMetrics() {
Help: desc,
}, []string{"endpoint", "role", "level", "title"})
GaugeMetricsMap[name] = *gaugeMetric
case "Percentile":
if _, ok := SummaryMetricsMap[name]; ok {
case "Percentile": //这个需要改动不能用这个表示,用gauge来表示分位数 --level(p50,p99),title(task_name)来替代区分
Copy link
Member

Choose a reason for hiding this comment

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

Use English because this is a globally open-source project.

@acelyc111
Copy link
Member

The PR title seems too long and incomplete.

…n and collection functions of partition entity metrics with new labels.
@github-actions github-actions bot added docs and removed docs labels Jan 5, 2024
@@ -25,7 +25,7 @@ meta_servers:
- 127.0.0.1:34603

# local server port
port : 34101
port : 8081
Copy link
Member

Choose a reason for hiding this comment

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

8080, 8081 are widely used ports for web servers, it would be better to use 34101 to keep compatible (old version of collectors may have been used by communituy users, don't change the behavior).

@@ -34,7 +34,7 @@ metrics:

prometheus:
# the exposed port for prometheus exposer
exposer_port : 1111
exposer_port : 8080
Copy link
Member

Choose a reason for hiding this comment

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

Same, and 34*01 ports are used by Pegasus by habit, how about use 34201 instead?

if err != nil {
return
}
}()

//Provide metrics for prometheus
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//Provide metrics for prometheus
// Provide metrics for Prometheus.

@limowang limowang closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants