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

Rename hcp-metrics-collector to consul-telemetry-collector #17327

Merged
merged 4 commits into from
May 16, 2023

Conversation

clly
Copy link
Contributor

@clly clly commented May 12, 2023

Description

We originally named the collector service name as hcp-metrics-collector. Rename the service name to follow the intended project name.

Also renamed the bind_socket_dir config key to remove the hcp reference. The collector can be used without HCP so it made sense to generalize the name

Testing & Reproduction steps

  • Unit tests have been modified to ensure everything passes.
  • Tested locally as follows:
  1. Run consul agent in dev mode: bin/consul agent -dev
  2. Register services:
  • web service: consul services register web.json
    {
    "service": {
        "name": "web",
        "port": 9191,
        "connect": {
            "sidecar_service": {
                "proxy": {
                    "config": {
                        "envoy_telemetry_collector_bind_socket_dir": "/Users/ashvitha.sridharan/tmp/"
                    }
                }
            }
        }
    }

}```

  • collector service: consul services register collector.json
{    "service":{
      "name": "consul-telemetry-collector",
      "port": 9090,
      "connect": {
          "sidecar_service": {
          }
      }
  }
}
  1. Run sidecars
  • ./bin/consul connect envoy -sidecar-for web -- -l debug
  • ./bin/consul connect envoy -admin-bind localhost:19001 -sidecar-for consul-telemetry-collector -- -l debug
  1. Run a mock metric handler (grpc service) and see metrics flow:
  2. Screenshot 2023-05-12 at 3 40 46 PM

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@clly clly added the backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. label May 12, 2023
@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support labels May 12, 2023
website/content/commands/connect/envoy.mdx Outdated Show resolved Hide resolved
@@ -190,6 +190,10 @@ the [`sidecar_service`](/consul/docs/connect/registration/sidecar-service) block
- `envoy_stats_flush_interval` - Configures Envoy's
[`stats_flush_interval`](https://www.envoyproxy.io/docs/envoy/v1.17.2/api-v3/config/bootstrap/v3/bootstrap.proto#envoy-v3-api-field-config-bootstrap-v3-bootstrap-stats-flush-interval).

- `envoy_telemetry_collector_bind_socket_dir` - Specifies the directory where Envoy creates a unix socket.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been the right place to have the docs, so the docs were moved from website/content/commands/connect/envoy.mdx to this file.

@Achooo Achooo force-pushed the f/metrics-collector-rename branch from 154e708 to df88393 Compare May 15, 2023 17:57
@Achooo Achooo force-pushed the f/metrics-collector-rename branch from df88393 to a98ea8b Compare May 15, 2023 18:02
@Achooo Achooo requested review from jjti, a team and wilkermichael and removed request for a team May 15, 2023 18:02
Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Minor suggestions for the docs.

Comment on lines 193 to 195
- `envoy_telemetry_collector_bind_socket_dir` - Specifies the directory where Envoy creates a unix socket.
Envoy sends metrics to the socket with which a consul telemetry collector can collect them.
The socket is not configured by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `envoy_telemetry_collector_bind_socket_dir` - Specifies the directory where Envoy creates a unix socket.
Envoy sends metrics to the socket with which a consul telemetry collector can collect them.
The socket is not configured by default.
- `envoy_telemetry_collector_bind_socket_dir` - Specifies the directory where Envoy creates a Unix socket.
Envoy sends metrics to the socket where a Consul telemetry collector can collect them.
The socket is not configured by default.

If I'm not mistaken, Unix is a proper noun.

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Docs LGTM!

Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@curtbushko curtbushko left a comment

Choose a reason for hiding this comment

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

LGTM!

You are re-naming everything and nothing is jumping out at me.

@Achooo Achooo merged commit 0789661 into main May 16, 2023
@Achooo Achooo deleted the f/metrics-collector-rename branch May 16, 2023 18:36
Achooo added a commit that referenced this pull request May 18, 2023
* Rename hcp-metrics-collector to consul-telemetry-collector

* Fix docs

* Fix doc comment

---------

Co-authored-by: Ashvitha Sridharan <[email protected]>
Achooo added a commit that referenced this pull request May 19, 2023
…17412)

* Rename hcp-metrics-collector to consul-telemetry-collector

* Fix docs

* Fix doc comment

---------

Co-authored-by: Connor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants