[receiver/apache]: add optional apache.server.name resource attribute #14926

aboguszewski-sumo · 2022-10-13T10:37:00Z

Description:
Server name is used as a normal attribute, but it should be a resource attribute. This PR does the first step of changing it, by adding a feature gate to use apache.server.name resource attribute instead of server_name metric-level attribute.

Link to tracking Issue: #14791

Testing:
Unit and integration tests.

Documentation:
mdatagen and changes to README.md

codeboten

@dmitryax as a breaking change for a "beta" receiver, should this be behind a feature gate? i recall that was the decision for the hostmetrics receiver.

dmitryax · 2022-10-15T00:01:57Z

@dmitryax as a breaking change for a "beta" receiver, should this be behind a feature gate? i recall that was the decision for the hostmetrics receiver.

I believe many backends flatten the attribute so it could've be ok to merge but the attribute name is changed which is a breaking change anyway. So I agree that it should be done through a feature gate.

@aboguszewski-sumo can you please update break the change into several steps using a feature gate:

add a disabled feature gate, add a warning if it's not enabled by the user.
enable feature gate by default
feature gate removed along with old code

this should span over several releases

cc @djaglowski as code owner

aboguszewski-sumo · 2022-10-17T07:13:21Z

@dmitryax sure, I can do this. However, I'm not sure if I understand how it should be done, so please correct me if I'm wrong:

add a resource attribute, but don't emit it by default; add a warning if disabled
emit the resource attribute by default
stop emitting the attribute server_name for each metric; remove the config option and warning

To sum up: server_name is still emitted always in points 1 and 2, gets removed in point 3. I can't imagine optional emitting metric-level attribute unless there's a configuration for mdatagen I'm not aware of, so I think that the only possibility is to remove it as late as possible.

Also - I want to add a mysql.server.port resource attribute, should it undergo a similar process? If not, can it get merged before we finish all steps for mysql.server.name?

aboguszewski-sumo · 2022-10-17T11:48:28Z

I have force pushed the changes for the first step on this branch (I have a backup of the original if needed). Please review if it's what you meant.

.chloggen/apache-server-name-add.yaml

receiver/apachereceiver/config.go

dmitryax · 2022-10-17T18:18:13Z

Also - I want to add a mysql.server.port resource attribute, should it undergo a similar process? If not, can it get merged before we finish all steps for mysql.server.name?

Removing server_name should be controlled by one feature gate. Adding mysql.server.port resource attribute should be controlled by another feature gate. Both of them should go through the same process:

introduced as disabled with a warning if not enabled by user
enabled by default
removed

About mdatagen:

you can move existing generated_metrics.go to deprecated_metrics.go
apply changes for enabled server_name feature gate, regenerate generated_metrics.go, remove duplicated API deprecated_metrics.go from, then you'll have both old and new APIs.
Then you can use both interfaces depending on whether the server_name feature gate is enabled or not.

aboguszewski-sumo · 2022-10-18T10:33:35Z

Sorry for the misunderstanding, I have pushed a version with feature gate.
Do I understand correctly that the transitions between feature gate states are done on every second release? (so, if this gets released in v0.63, then it will be enabled by default in v0.65)

dmitryax · 2022-10-18T16:33:18Z

Do I understand correctly that the transitions between feature gate states are done on every second release? (so, if this gets released in v0.63, then it will be enabled by default in v0.65)

Yes, that's an ideal approach

dmitryax · 2022-10-18T16:37:45Z

receiver/apachereceiver/config.go

@@ -28,6 +28,7 @@ type Config struct {
 	scraperhelper.ScraperControllerSettings `mapstructure:",squash"`
 	confighttp.HTTPClientSettings           `mapstructure:",squash"`
 	serverName                              string
+	emitServerNameAsResourceAttribute       bool


This is still a config option, not a feature gate. Please take a look at this feature gates example

opentelemetry-collector-contrib/receiver/zookeeperreceiver/scraper.go

Line 45 in 6457705

emitMetricsWithDirectionAttributeFeatureGate = featuregate.Gate{

There is a feature gate used, just not directly in the scraper, but in the factory:
https://github.com/aboguszewski-sumo/opentelemetry-collector-contrib/blob/6bac04c5014dbda6dfe8a424c02b9ab93459cbcc/receiver/apachereceiver/factory.go#L38-L50
I just thought that a pattern where it is injected in the scraper with the config seems more reasonable. I'll just move this code to the scraper then.

djaglowski

@dmitryax, I agree w/ your suggestions here.

Since there is some ongoing work, I invalidated your previous approval to make sure this doesn't get merged prematurely.

dmitryax

LGTM. Just few nits

receiver/apachereceiver/internal/metadata/deprecated_metrics.go

dmitryax · 2022-10-21T00:28:32Z

receiver/apachereceiver/scraper.go

 	}
+
+	if !a.emitMetricsWithServerNameAsResourceAttribute {
+		settings.Logger.Warn(fmt.Sprintf("Feature gate %s is not enabled. Please see the README.md file of apache receiver for more information.", EmitServerNameAsResourceAttribute))


Maybe just put a link to README here?

I added the link. However, I was not sure which branch/tag should we link to - I picked main, to avoid predicting if the next stable version will be v0.63.0 or some other v0.63.x like in case of v0.57.

receiver/apachereceiver/scraper.go

djaglowski

LGTM

aboguszewski-sumo · 2022-10-27T09:34:49Z

This did not manage to be merged before v0.63, so I updated information about turning this feature on by default in the readme.

@dmitryax any chance of merging this soon?

aboguszewski-sumo · 2022-11-03T12:19:46Z

@djaglowski @dmitryax what's the status of this PR? I believe I've made all necessary changes.

…open-telemetry#14926) * feat: add feature gate with default path

aboguszewski-sumo requested a review from a team October 13, 2022 10:37

aboguszewski-sumo requested a review from djaglowski as a code owner October 13, 2022 10:37

github-actions bot assigned dmitryax Oct 13, 2022

aboguszewski-sumo force-pushed the apache-name-resource-attr branch from 5d98e89 to 49752c4 Compare October 13, 2022 10:41

aboguszewski-sumo mentioned this pull request Oct 13, 2022

[receiver/apache] send port number as an attribute #14791

Closed

dmitryax approved these changes Oct 14, 2022

View reviewed changes

codeboten reviewed Oct 14, 2022

View reviewed changes

aboguszewski-sumo force-pushed the apache-name-resource-attr branch from 49752c4 to a376537 Compare October 17, 2022 11:43

aboguszewski-sumo changed the title ~~[receiver/apache]: extract server name as resource attribute~~ [receiver/apache]: add optional apache.server.name resource attribute Oct 17, 2022

aboguszewski-sumo commented Oct 17, 2022

View reviewed changes

.chloggen/apache-server-name-add.yaml Outdated Show resolved Hide resolved

dmitryax reviewed Oct 17, 2022

View reviewed changes

receiver/apachereceiver/config.go Outdated Show resolved Hide resolved

aboguszewski-sumo added 2 commits October 18, 2022 12:00

feat: add feature gate with default path

34f3348

feat: add path for enabled feature gate

2dac70f

aboguszewski-sumo force-pushed the apache-name-resource-attr branch 3 times, most recently from 134e6f9 to c615a18 Compare October 18, 2022 10:15

aboguszewski-sumo added 2 commits October 18, 2022 12:20

chore: move checking the feature gate to the factory

2a4c28f

docs: add info about feature gate to the readme

6bac04c

aboguszewski-sumo force-pushed the apache-name-resource-attr branch from c615a18 to 6bac04c Compare October 18, 2022 10:20

aboguszewski-sumo requested a review from dmitryax October 18, 2022 11:19

dmitryax reviewed Oct 18, 2022

View reviewed changes

djaglowski requested a review from dmitryax October 18, 2022 16:41

djaglowski requested changes Oct 18, 2022

View reviewed changes

chore: move the feature gate from factory to scraper

825ebe4

aboguszewski-sumo force-pushed the apache-name-resource-attr branch from 40161d6 to 825ebe4 Compare October 20, 2022 12:42

dmitryax reviewed Oct 21, 2022

View reviewed changes

aboguszewski-sumo requested a review from dmitryax October 21, 2022 07:41

chore: add readme link and refactor code

3ac9b82

aboguszewski-sumo force-pushed the apache-name-resource-attr branch from 8c12baf to 3ac9b82 Compare October 21, 2022 08:01

chore: fix method names

00a9260

djaglowski approved these changes Oct 24, 2022

View reviewed changes

chore: change readme

ff683d4

djaglowski merged commit d420dbc into open-telemetry:main Nov 3, 2022

dineshg13 pushed a commit to DataDog/opentelemetry-collector-contrib that referenced this pull request Nov 21, 2022

[receiver/apache]: Add optional apache.server.name resource attribute (…

d3527a0

…open-telemetry#14926) * feat: add feature gate with default path

shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022

[receiver/apache]: Add optional apache.server.name resource attribute (…

15a9f27

…open-telemetry#14926) * feat: add feature gate with default path

lootek mentioned this pull request Feb 23, 2023

[receiver/hostmetrics] Add available memory state lootek/opentelemetry-collector-contrib#1

Closed

plantfansam mentioned this pull request Jul 21, 2023

disregard #24427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/apache]: add optional apache.server.name resource attribute #14926

[receiver/apache]: add optional apache.server.name resource attribute #14926

aboguszewski-sumo commented Oct 13, 2022 •

edited

Loading

codeboten left a comment

dmitryax commented Oct 15, 2022

aboguszewski-sumo commented Oct 17, 2022 •

edited

Loading

aboguszewski-sumo commented Oct 17, 2022

dmitryax commented Oct 17, 2022

aboguszewski-sumo commented Oct 18, 2022

dmitryax commented Oct 18, 2022 •

edited

Loading

dmitryax Oct 18, 2022

aboguszewski-sumo Oct 20, 2022

aboguszewski-sumo Oct 20, 2022

djaglowski left a comment

dmitryax left a comment

dmitryax Oct 21, 2022

aboguszewski-sumo Oct 21, 2022

djaglowski left a comment

aboguszewski-sumo commented Oct 27, 2022

aboguszewski-sumo commented Nov 3, 2022

[receiver/apache]: add optional apache.server.name resource attribute #14926

[receiver/apache]: add optional apache.server.name resource attribute #14926

Conversation

aboguszewski-sumo commented Oct 13, 2022 • edited Loading

codeboten left a comment

Choose a reason for hiding this comment

dmitryax commented Oct 15, 2022

aboguszewski-sumo commented Oct 17, 2022 • edited Loading

aboguszewski-sumo commented Oct 17, 2022

dmitryax commented Oct 17, 2022

aboguszewski-sumo commented Oct 18, 2022

dmitryax commented Oct 18, 2022 • edited Loading

dmitryax Oct 18, 2022

Choose a reason for hiding this comment

aboguszewski-sumo Oct 20, 2022

Choose a reason for hiding this comment

aboguszewski-sumo Oct 20, 2022

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

dmitryax left a comment

Choose a reason for hiding this comment

dmitryax Oct 21, 2022

Choose a reason for hiding this comment

aboguszewski-sumo Oct 21, 2022

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

aboguszewski-sumo commented Oct 27, 2022

aboguszewski-sumo commented Nov 3, 2022

aboguszewski-sumo commented Oct 13, 2022 •

edited

Loading

aboguszewski-sumo commented Oct 17, 2022 •

edited

Loading

dmitryax commented Oct 18, 2022 •

edited

Loading