Cloud Foundry metrics receiver #1 - config, factory, docs #4626

agoallikmaa · 2021-08-16T11:09:28Z

Description: Adds a Cloud Foundry metric receiver which reads metrics from Cloud Foundry Reverse Log Proxy Gateway. More details available in the README.md. make gotidy seems to have made plenty of subtle changes to go.sum files, not sure if this is normal. This PR contains the overall structure, documentation, implementation for config and factory, but does NOT contain the implementation of the receiver and does not enable the component, as that will come in separate PRs later.

Link to tracking Issue: #5320

Testing: Unit tests. Manual testing was performed against Tanzu Application Service (TAS) versions 2.7, 2.8 and 2.11. Considered adding an integration test with mocked HTTP servers acting as endpoints where the HTTP server would provide a constant response (prerecorded from the real TAS traffic), but not sure if mocks would make more sense.

Documentation: README.md and doc.go for the new receiver module were added.

bogdandrutu · 2021-08-17T19:38:34Z

Please follow the steps:
https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#when-adding-a-new-component

agoallikmaa · 2021-08-23T11:00:30Z

Updated current PR to be the first stage PR of adding a new component by removing concrete implementation of everything except factory and config, and removing from components.go.

bogdandrutu · 2021-08-23T20:52:20Z

Please identify couple of people who will maintain this component and add them to the CODEOWNERS

github-actions · 2021-09-07T05:15:16Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

agoallikmaa · 2021-09-07T09:18:59Z

Code owners present, rebased.

github-actions · 2021-09-15T05:15:22Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

pellared

Nice job. I added some minor comments which should be easy to address.

Can you edit the description of this PR to define what this PR contains?

Can you please create an issue e.g. named Cloud Foundry metrics receiver? We could use it and use it to track the cloudfoundryreceiver implementation status. From https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#before-you-start:

If you would like to work on something that is not listed as an issue [...], then create an issue and describe your proposal. It is best to do this in advance so that maintainers can decide if the proposal is a good fit for this repository. This will help avoid situations when you spend significant time on something that maintainers may decide this repo is not the right place for.

pellared · 2021-09-20T07:04:18Z

receiver/cloudfoundryreceiver/README.md

+## Configuration
+
+The receiver takes the following configuration options:
+* `rlp_gateway_url` - URL of the RLP gateway, required, typically `https://log-stream.<cf-system-domain>`
+* `rlp_gateway_skip_tls_verify` - whether to skip TLS verify for the RLP Gateway endpoint, default `false`
+* `rlp_gateway_shard_id` - metrics are load balanced among receivers that use the same shard ID, therefore this must 
+  only be set if there are multiple receivers which must both receive all the metrics instead of them being balanced
+  between them. Default value is `opentelemetry`
+* `uaa_url` - URL of the UAA provider, required, typically `https://uaa.<cf-system-domain>`
+* `uaa_skip_tls_verify` - whether to skip TLS verify for the UAA endpoint, default `false`
+* `uaa_username` - name of the UAA user (required grant types/authorities described above)
+* `uaa_username` - password of the UAA user
+* `http_timeout` - HTTP socket timeout used for RLP Gateway, default `10s`


Can you format it to make it more readable? It should be easy to find if an option is required and what is its default value.

E.g. https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/filelogreceiver#configuration

please resolve

pellared · 2021-09-20T07:06:48Z

receiver/cloudfoundryreceiver/README.md

+The following is an example sequence of commands to create the UAA user using the `uaac` command line utility:
+* `uaac target https://uaa.<cf-system-domain>`


nit: markdownlint wants to have a blank line between a paragraph and a list

nit: the same comment applies to other places in the README.md

pellared · 2021-09-20T07:15:12Z

receiver/cloudfoundryreceiver/config.go

+
+	_, err := url.Parse(value)
+	if err != nil {
+		return fmt.Errorf("failed to parse %s value %s as url: %v", name, value, err)


There is no need to pass the value. See https://play.golang.org/p/XcW50z01QNf

Suggested change

return fmt.Errorf("failed to parse %s value %s as url: %v", name, value, err)

return fmt.Errorf("parse %s as URL: %v", name, err)

please resolve

pellared · 2021-09-20T07:18:18Z

receiver/cloudfoundryreceiver/config_test.go

+
+func TestLoadConfig(t *testing.T) {
+	factories, err := componenttest.NopFactories()
+	assert.Nil(t, err)


Suggested change

assert.Nil(t, err)

require.NoError(t, err)

please resolve

pellared · 2021-09-20T07:23:24Z

receiver/cloudfoundryreceiver/config_test.go

+	"go.opentelemetry.io/collector/config/configtest"
+)
+
+func TestLoadConfig(t *testing.T) {


I suggest adding a TODO for the next PR: add negative tests e.g. in TestFailedLoadConfig

please resolve

pellared · 2021-09-20T07:29:01Z

receiver/cloudfoundryreceiver/config_test.go

+	require.NoError(t, err)
+	require.NotNil(t, cfg)
+
+	assert.Equal(t, len(cfg.Receivers), 2)


How about changing this line to:

Suggested change

assert.Equal(t, len(cfg.Receivers), 2)

require.Len(t, cfg.Receivers, 2)

so that it does not panic later if the length would be 0 or 1?

please resolve

pellared · 2021-09-20T07:33:15Z

receiver/cloudfoundryreceiver/config_test.go

+	assert.Equal(t, len(cfg.Receivers), 2)
+
+	r0 := cfg.Receivers[config.NewID(typeStr)]
+	assert.Equal(t, r0, factory.CreateDefaultConfig())


Expected is first, actual is second.

Suggested change

assert.Equal(t, r0, factory.CreateDefaultConfig())

assert.Equal(t, factory.CreateDefaultConfig(), r0)

See: https://pkg.go.dev/github.com/stretchr/[email protected]/assert#Equal

please resolve

pellared · 2021-09-20T07:34:02Z

receiver/cloudfoundryreceiver/config_test.go

+	assert.Equal(t, r1,
+		&Config{
+			ReceiverSettings:        config.NewReceiverSettings(config.NewIDWithName(typeStr, "one")),
+			RLPGatewayURL:           "https://log-stream.sys.example.internal",
+			RLPGatewaySkipTLSVerify: true,
+			RLPGatewayShardID:       "otel-test",
+			UAAUrl:                  "https://uaa.sys.example.internal",
+			UAASkipTLSVerify:        true,
+			UAAUsername:             "admin",
+			UAAPassword:             "test",
+			HTTPTimeout:             time.Second * 20,
+		})


Expected is first, actual is second.

See: https://pkg.go.dev/github.com/stretchr/[email protected]/assert#Equal

please resolve

pellared · 2021-09-20T07:36:33Z

receiver/cloudfoundryreceiver/factory_test.go

+func TestCreateDefaultConfig(t *testing.T) {
+	factory := NewFactory()
+	cfg := factory.CreateDefaultConfig()
+	assert.NotNil(t, cfg, "failed to create default config")


Should it not be require? Does the next line make any sense if this one fails?

please resolve

pellared · 2021-09-20T07:51:47Z

Considered adding an integration test with mocked HTTP servers acting as endpoints where the HTTP server would provide a constant response (prerecorded from the real TAS traffic), but not sure if mocks would make more sense.

For me, it makes a lot of sense, because:

testing with TAS is very time-consuming and this may extremely help during refactoring;
better to have a test with prerecorded traffic than no test;
not everyone has access to TAS.

agoallikmaa · 2021-09-21T15:19:39Z

Can you edit the description of this PR to define what this PR contains?

Added an additional sentence to description that explains what this PR includes and what it does not.

Can you please create an issue e.g. named Cloud Foundry metrics receiver? We could use it and use it to track the cloudfoundryreceiver implementation status. From https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#before-you-start:

Issue created, edited PR description to link to the issue.

pellared

LGTM

tigrannajaryan · 2021-09-21T18:38:11Z

receiver/cloudfoundryreceiver/README.md

+### Attributes
+
+All the metrics have the following attributes:
+* `origin` - origin name as documented by Cloud Foundry


Is origin both an attribute and also prepended to the metric name? (I don't know if this is desirable or no, just double checking that the docs are correct).

tigrannajaryan · 2021-09-21T18:39:16Z

receiver/cloudfoundryreceiver/README.md

+* `origin` - origin name as documented by Cloud Foundry
+* `source` - for applications, the GUID of the application, otherwise equal to `origin`
+
+For CF/TAS deployed in BOSH, the following attributes are also present, using their canonical BOSH meanings:


Nit: it may be useful to link abbreviations to their definitions somewhere. I don't know what TAS or BOSH and being able to quickly find out can help maintaining the component in the future.

tigrannajaryan · 2021-09-21T18:41:51Z

receiver/cloudfoundryreceiver/config.go

+	RLPGatewaySkipTLSVerify bool          `mapstructure:"rlp_gateway_skip_tls_verify"`
+	RLPGatewayShardID       string        `mapstructure:"rlp_gateway_shard_id"`
+	UAAUrl                  string        `mapstructure:"uaa_url"`
+	UAASkipTLSVerify        bool          `mapstructure:"uaa_skip_tls_verify"`


Consider using the generic TLSClientSetting (if this receiver is capable of supporting the settings). Otherwise it would be still good to use the same names for settings (e.g. insecure_skip_verify).

tigrannajaryan · 2021-09-21T18:42:11Z

receiver/cloudfoundryreceiver/config.go

+	RLPGatewayURL           string        `mapstructure:"rlp_gateway_url"`
+	RLPGatewaySkipTLSVerify bool          `mapstructure:"rlp_gateway_skip_tls_verify"`
+	RLPGatewayShardID       string        `mapstructure:"rlp_gateway_shard_id"`


Would it be nicer to make rtl a section with subkeys?

tigrannajaryan · 2021-09-21T18:42:28Z

receiver/cloudfoundryreceiver/config.go

+	UAAUrl                  string        `mapstructure:"uaa_url"`
+	UAASkipTLSVerify        bool          `mapstructure:"uaa_skip_tls_verify"`
+	UAAUsername             string        `mapstructure:"uaa_username"`
+	UAAPassword             string        `mapstructure:"uaa_password"`


Would it be nicer to make uaa a section with subkeys?

tigrannajaryan · 2021-09-21T18:43:12Z

receiver/cloudfoundryreceiver/config.go

+	UAASkipTLSVerify        bool          `mapstructure:"uaa_skip_tls_verify"`
+	UAAUsername             string        `mapstructure:"uaa_username"`
+	UAAPassword             string        `mapstructure:"uaa_password"`
+	HTTPTimeout             time.Duration `mapstructure:"http_timeout"`


Consider using HTTPClientSettings which already includes timeout setting.

tigrannajaryan · 2021-09-21T18:44:54Z

I am going to merge this now since the component is not enabled yet. Please address the comments I left in the future PRs.

Adds implementation to the Cloud Foundry receiver skeleton merged in #4626 . **Link to tracking Issue:** #5320 **Testing:** Some implementation parts have been unit tested, the rest will be added as integration tests, in a separate PR which tests against a local HTTP server which will respond with prerecorded responses (recorded from running it against Tanzu Application Service). **Documentation:** Configuration was updated in documentation to reflect using `HTTPClientSettings` instead of custom fields.

agoallikmaa requested review from Aneurysm9, anuraaga and kbrockhoff as code owners August 16, 2021 11:09

agoallikmaa requested a review from a team August 16, 2021 11:09

github-actions bot assigned kbrockhoff Aug 16, 2021

agoallikmaa force-pushed the cloudfoundry-receiver branch from f5c47d1 to 5c269e6 Compare August 23, 2021 10:57

agoallikmaa changed the title ~~Cloud Foundry metrics receiver~~ Cloud Foundry metrics receiver - config, factory, docs Aug 23, 2021

agoallikmaa changed the title ~~Cloud Foundry metrics receiver - config, factory, docs~~ Cloud Foundry metrics receiver #1 - config, factory, docs Aug 23, 2021

github-actions bot added the Stale label Sep 7, 2021

agoallikmaa added 7 commits September 7, 2021 11:59

Cloud Foundry receiver

ce06827

Keep only parts required for initial PR

372b7fe

gotidy after rebase

41e017e

Remove cloudfoundry as an enabled component for now

bc4b7a7

Fix unused import

0eed6dc

Added code owners for cloudfoundryreceiver

d744688

gotidy after rebase #2

2780141

agoallikmaa force-pushed the cloudfoundry-receiver branch from f21b7b3 to 2780141 Compare September 7, 2021 09:18

github-actions bot removed the Stale label Sep 8, 2021

github-actions bot added the Stale label Sep 15, 2021

pellared reviewed Sep 20, 2021

View reviewed changes

github-actions bot removed the Stale label Sep 21, 2021

agoallikmaa added 2 commits September 21, 2021 17:41

Addressed PR comments

340bd01

Added a todo

22764b0

agoallikmaa mentioned this pull request Sep 21, 2021

Implement Cloud Foundry metrics receiver #5320

Closed

pellared approved these changes Sep 21, 2021

View reviewed changes

tigrannajaryan reviewed Sep 21, 2021

View reviewed changes

tigrannajaryan approved these changes Sep 21, 2021

View reviewed changes

tigrannajaryan merged commit ceec9c7 into open-telemetry:main Sep 21, 2021

agoallikmaa mentioned this pull request Oct 1, 2021

Cloud Foundry receiver step 2 - implementation #5543

Merged

		The following is an example sequence of commands to create the UAA user using the `uaac` command line utility:
		* `uaac target https://uaa.<cf-system-domain>`

	return fmt.Errorf("failed to parse %s value %s as url: %v", name, value, err)
	return fmt.Errorf("parse %s as URL: %v", name, err)

	assert.Equal(t, len(cfg.Receivers), 2)
	require.Len(t, cfg.Receivers, 2)

	assert.Equal(t, r0, factory.CreateDefaultConfig())
	assert.Equal(t, factory.CreateDefaultConfig(), r0)

Cloud Foundry metrics receiver #1 - config, factory, docs #4626

Cloud Foundry metrics receiver #1 - config, factory, docs #4626

Conversation

agoallikmaa commented Aug 16, 2021 • edited Loading

bogdandrutu commented Aug 17, 2021

agoallikmaa commented Aug 23, 2021

bogdandrutu commented Aug 23, 2021

github-actions bot commented Sep 7, 2021

agoallikmaa commented Sep 7, 2021

github-actions bot commented Sep 15, 2021

pellared left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared commented Sep 20, 2021 • edited Loading

agoallikmaa commented Sep 21, 2021

pellared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Sep 21, 2021

agoallikmaa commented Aug 16, 2021 •

edited

Loading

pellared left a comment •

edited

Loading

pellared commented Sep 20, 2021 •

edited

Loading