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

[receiver/apache] send port as a resource attribute #16053

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .chloggen/apache-port-resource-attr.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: apachereceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: add port resource attribute

# One or more tracking issues related to the change
issues: [14791]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
10 changes: 10 additions & 0 deletions receiver/apachereceiver/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,13 @@ This is considered a breaking change for existing users of this receiver, and it
This feature gate will eventually be enabled by default, and eventually the old implementation will be removed. It aims
to give users time to migrate to the new implementation. The target release for this featuregate to be enabled by default
is 0.66.0.

**ALPHA**: `receiver.apache.emitPortAsResourceAttribute`

The feature gate `receiver.apache.emitPortAsResourceAttribute` once enabled starts emitting the metrics with a resource attribute `apache.server.port`.

This is considered a breaking change for existing users of this receiver, and it is recommended to migrate to the new implementation when possible. Any new users planning to adopt this receiver should enable this feature gate to avoid having to migrate any visualisations or alerts.

This feature gate will eventually be enabled by default, and eventually the old implementation will be removed. It aims
to give users time to migrate to the new implementation. The target release for this featuregate to be enabled by default
is 0.66.0.
15 changes: 15 additions & 0 deletions receiver/apachereceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Config struct {
scraperhelper.ScraperControllerSettings `mapstructure:",squash"`
confighttp.HTTPClientSettings `mapstructure:",squash"`
serverName string
port string
Metrics metadata.MetricsSettings `mapstructure:"metrics"`
}

Expand All @@ -37,6 +38,9 @@ var (
defaultPort = "8080"
defaultPath = "server-status"
defaultEndpoint = fmt.Sprintf("%s%s:%s/%s?auto", defaultProtocol, defaultHost, defaultPort, defaultPath)

httpDefaultPort = "80"
httpsDefaultPort = "443"
)

func (cfg *Config) Validate() error {
Expand All @@ -55,5 +59,16 @@ func (cfg *Config) Validate() error {
}

cfg.serverName = u.Hostname()
cfg.port = u.Port()

if cfg.port == "" {
if u.Scheme == "https" {
cfg.port = httpsDefaultPort
} else if u.Scheme == "http" {
cfg.port = httpDefaultPort
}
Copy link
Member

Choose a reason for hiding this comment

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

The Validate function should not change the configuration.

I suggest moving the port field to the scraper and setting it in the factory.

Copy link
Member Author

@aboguszewski-sumo aboguszewski-sumo Nov 3, 2022

Choose a reason for hiding this comment

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

I understand the reason, but serverName is set in this function anyway - shouldn't it also be changed?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, really it should. I think it's an inconsequential change but is better for the sake of consistency, so it's fine to do in this PR in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have caught a bug here.
After the fix, the integration tests stopped passing - the expected value of serverName was "", while the receiver returned "localhost". I checked immediately - and it happens that Validate is called only inside unit tests - so outside the tests, serverName wasn't actually set and was always an empty string.
I fixed this and pushed the changes on this branch.

// else: unknown scheme, leave port as empty string
}

return nil
}
49 changes: 49 additions & 0 deletions receiver/apachereceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,52 @@ func TestLoadConfig(t *testing.T) {

require.Equal(t, expected, cfg)
}

func TestPortValidate(t *testing.T) {
testCases := []struct {
desc string
endpoint string
expectedPort string
}{
{
desc: "http with specified port",
endpoint: "http://localhost:8080/server-status?auto",
expectedPort: "8080",
},
{
desc: "http without specified port",
endpoint: "http://localhost/server-status?auto",
expectedPort: "80",
},
{
desc: "https with specified port",
endpoint: "https://localhost:8080/server-status?auto",
expectedPort: "8080",
},
{
desc: "https without specified port",
endpoint: "https://localhost/server-status?auto",
expectedPort: "443",
},
{
desc: "unknown protocol with specified port",
endpoint: "abc://localhost:8080/server-status?auto",
expectedPort: "8080",
},
{
desc: "port unresolvable",
endpoint: "abc://localhost/server-status?auto",
expectedPort: "",
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
cfg := NewFactory().CreateDefaultConfig().(*Config)
cfg.Endpoint = tc.endpoint
err := cfg.Validate()

require.NoError(t, err)
require.Equal(t, tc.expectedPort, cfg.port)
})
}
}
1 change: 1 addition & 0 deletions receiver/apachereceiver/documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ metrics:
| Name | Description | Type |
| ---- | ----------- | ---- |
| apache.server.name | The name of the Apache HTTP server. | Str |
| apache.server.port | The port of the Apache HTTP server. | Str |

## Metric attributes

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions receiver/apachereceiver/metadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ resource_attributes:
apache.server.name:
description: The name of the Apache HTTP server.
type: string
apache.server.port:
description: The port of the Apache HTTP server.
type: string

attributes:
workers_state:
Expand Down
20 changes: 20 additions & 0 deletions receiver/apachereceiver/scraper.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
const (
readmeURL = "https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/apachereceiver/README.md"
EmitServerNameAsResourceAttribute = "receiver.apache.emitServerNameAsResourceAttribute"
EmitPortAsResourceAttribute = "receiver.apache.emitPortAsResourceAttribute"
featureGateWarning = "Feature gate %s is not enabled. Please see the README.md file of apache receiver for more information."
)

var (
Expand All @@ -46,10 +48,16 @@ var (
Description: "When enabled, the name of the server will be sent as an apache.server.name resource attribute " +
"instead of a metric-level server_name attribute.",
}
emitPortAsResourceAttribute = featuregate.Gate{
ID: EmitPortAsResourceAttribute,
Enabled: false,
Description: "When enabled, the port of the server will be sent as an apache.server.name resource attribute.",
}
)

func init() {
featuregate.GetRegistry().MustRegister(emitServerNameAsResourceAttribute)
featuregate.GetRegistry().MustRegister(emitPortAsResourceAttribute)
}

type apacheScraper struct {
Expand All @@ -60,6 +68,7 @@ type apacheScraper struct {

// Feature gates regarding resource attributes
emitMetricsWithServerNameAsResourceAttribute bool
emitMetricsWithPortAsResourceAttribute bool
}

func newApacheScraper(
Expand All @@ -71,6 +80,7 @@ func newApacheScraper(
cfg: cfg,
mb: metadata.NewMetricsBuilder(cfg.Metrics, settings.BuildInfo),
emitMetricsWithServerNameAsResourceAttribute: featuregate.GetRegistry().IsEnabled(EmitServerNameAsResourceAttribute),
emitMetricsWithPortAsResourceAttribute: featuregate.GetRegistry().IsEnabled(EmitPortAsResourceAttribute),
}

if !a.emitMetricsWithServerNameAsResourceAttribute {
Expand All @@ -79,6 +89,12 @@ func newApacheScraper(
)
}

if !a.emitMetricsWithPortAsResourceAttribute {
settings.Logger.Warn(
fmt.Sprintf("Feature gate %s is not enabled. Please see the README for more information: %s", EmitPortAsResourceAttribute, readmeURL),
)
}

return a
}

Expand Down Expand Up @@ -111,6 +127,10 @@ func (r *apacheScraper) scrape(context.Context) (pmetric.Metrics, error) {
err = r.scrapeWithServerNameAttr(stats)
}

if r.emitMetricsWithPortAsResourceAttribute {
emitWith = append(emitWith, metadata.WithApacheServerPort(r.cfg.port))
}

return r.mb.Emit(emitWith...), err
}

Expand Down
9 changes: 7 additions & 2 deletions receiver/apachereceiver/scraper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"path/filepath"
"testing"

Expand All @@ -41,8 +42,7 @@ func TestScraper(t *testing.T) {
require.NoError(t, cfg.Validate())

// Let this test check if it works with the feature enabled and the integration test will test the feature disabled.
err := featuregate.GetRegistry().Apply(map[string]bool{EmitServerNameAsResourceAttribute: true})

err := featuregate.GetRegistry().Apply(map[string]bool{EmitServerNameAsResourceAttribute: true, EmitPortAsResourceAttribute: true})
require.NoError(t, err)

scraper := newApacheScraper(componenttest.NewNopReceiverCreateSettings(), cfg)
Expand All @@ -56,7 +56,12 @@ func TestScraper(t *testing.T) {
expectedFile := filepath.Join("testdata", "scraper", "expected.json")
expectedMetrics, err := golden.ReadMetrics(expectedFile)
require.NoError(t, err)
url, err := url.Parse(apacheMock.URL)
require.NoError(t, err)
expectedMetrics.ResourceMetrics().At(0).Resource().Attributes().Remove("apache.server.port")
aboguszewski-sumo marked this conversation as resolved.
Show resolved Hide resolved
expectedMetrics.ResourceMetrics().At(0).Resource().Attributes().PutStr("apache.server.port", url.Port())

// The port is random, so we shouldn't check if this value matches.
require.NoError(t, scrapertest.CompareMetrics(expectedMetrics, actualMetrics))
}

Expand Down
8 changes: 7 additions & 1 deletion receiver/apachereceiver/testdata/scraper/expected.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@
"value": {
"stringValue": "127.0.0.1"
}
}
},
{
"key": "apache.server.port",
"value": {
"stringValue": "8080"
}
}
]
},
"scopeMetrics": [
Expand Down