Skip to content

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

Merged
Achooo merged 4 commits into
mainfrom
f/metrics-collector-rename
May 16, 2023
Merged

Rename hcp-metrics-collector to consul-telemetry-collector#17327
Achooo merged 4 commits into
mainfrom
f/metrics-collector-rename

Conversation

@clly
Copy link
Copy Markdown
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

@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
@clly clly requested review from Achooo, erichaberkorn and freddygv May 12, 2023 15:56
Comment thread agent/proxycfg/connect_proxy.go Outdated
Comment thread command/connect/envoy/bootstrap_config.go Outdated
@Achooo Achooo force-pushed the f/metrics-collector-rename branch from 384da30 to 1e08e3d Compare May 12, 2023 18:36
@Achooo Achooo marked this pull request as ready for review May 12, 2023 19:43
@Achooo Achooo requested a review from a team as a code owner May 12, 2023 19:43
@Achooo Achooo requested a review from freddygv May 12, 2023 19:48
@Achooo Achooo requested review from a team and curtbushko and removed request for a team May 12, 2023 21:02
@Achooo Achooo removed the request for review from erichaberkorn May 15, 2023 16:33
Comment thread website/content/commands/connect/envoy.mdx Outdated
Copy link
Copy Markdown
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
Comment thread website/content/docs/connect/proxies/envoy.mdx Outdated
@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 a team, jjti and wilkermichael and removed request for a team May 15, 2023 18:02
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <ashvitha.sridharan@hashicorp.com>
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 <connor.kelly@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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