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

[exporter-helper] [resource_to_label_conversion] add helper function and expose exporter settings #2060

Merged
merged 10 commits into from
Nov 9, 2020

Conversation

hossain-rayhan
Copy link
Contributor

Description:
We need a common helper function which can convert resource attributes to metric labels and be utilized by every exporter. In the linked issue, @bogdandrutu suggested two possible ways- a consumer or a helper function. In this PR, we have both of the consumer and helper function ready. However, I was only able to utilize the helper function for end to end test modifying the logging exporter. Every exporter can call this helper function before exporting the metrics to destination.

For the consumer setup, I cannot find a place to plug in this in between a processor and an exporter. I think I need more guidance on how we can make it a common utility for all exporters. Thanks in advance.

Link to tracking Issue:
Issue 1359

Testing:
End_To_End Test: Tested manually with awsecscontainermetrics receiver and logging exporter.
Unit test added.

@hossain-rayhan hossain-rayhan requested a review from a team November 4, 2020 18:41
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #2060 (fee5a75) into master (c6b8f28) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2060      +/-   ##
==========================================
+ Coverage   92.10%   92.11%   +0.01%     
==========================================
  Files         278      279       +1     
  Lines       16978    17038      +60     
==========================================
+ Hits        15637    15695      +58     
- Misses        922      923       +1     
- Partials      419      420       +1     
Impacted Files Coverage Δ
exporter/exporterhelper/common.go 100.00% <100.00%> (ø)
exporter/exporterhelper/metricshelper.go 86.84% <100.00%> (+0.73%) ⬆️
exporter/exporterhelper/resource_to_label.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 91.54% <0.00%> (-2.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6b8f28...fee5a75. Read the comment docs.

@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu can you share your views here regarding the next steps. As discussed yesterdays Collector SIG meeting, the core function is working fine. I need guidance regarding making it a common utility for all exporters.

@hossain-rayhan
Copy link
Contributor Author

hossain-rayhan commented Nov 6, 2020

Update
[1] Now the helper function and the consumer clone the incoming pdata.Metrics and modify the copy.
[2] ResourceToLabel setting option is exposed from exporterhelper so that all the exporters can utilize it.

TODO
Now we have access to ResourceToLabelSettings.Enabled value (true or false- which user can configure in OpenTelemetry config file). Need to write some kind of mechanism to use our ConvertResourceToLabels () helper function if this option is enabled.

Help Needed
From the exporterhelper, I cannot see a way to get access to pdata.Metrics to modify (the copy) and pass it to the next consumer. Maybe I am missing something. Working on it. If I cannot figure it out, I would highly appreciate any help/guidance here @bogdandrutu .

Thanks.

exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
exporter/exporterhelper/common.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

One option (needs some refactoring) you can do it in metricshelper.ConsumeMetrics:

type metricsExporter struct {
	*baseExporter
	consumer.MetricsConsumer
}

// Delete the current ConsumeMetrics from metricsExporter

// NewMetricsExporter creates an MetricsExporter that records observability metrics and wraps every request with a Span.
func NewMetricsExporter(
	cfg configmodels.Exporter,
	logger *zap.Logger,
	pushMetricsData PushMetricsData,
	options ...ExporterOption,
) (component.MetricsExporter, error) {

        .......

	rc := &requestConsumer{
		sender: be.sender,
		pusher: pushMetricsData,
	}

        // Can plugin the resource consumer here if enabled.

	cc := &contextConsumer{
		name: be.cfg.Name(),
		next: rc,
	}

	return &metricsExporter{
		baseExporter:    be,
		MetricsConsumer: cc,
	}, nil
}


type contextConsumer struct {
	name string
	next consumer.MetricsConsumer
}

func (cc *contextConsumer) ConsumeMetrics(ctx context.Context, md pdata.Metrics) error {
	return cc.next.ConsumeMetrics(obsreport.ExporterContext(ctx, cc.name), md)
}

type requestConsumer struct {
	sender requestSender
	pusher PushMetricsData
}

func (rc *requestConsumer) ConsumeMetrics(ctx context.Context, md pdata.Metrics) error {
	req := newMetricsRequest(ctx, md, rc.pusher)
	_, err := rc.sender.send(req)
	return err
}

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Your option also works, I think requires less work, so even better. We can cleanup later the consumer chain as I proposed.

exporter/exporterhelper/metricshelper.go Outdated Show resolved Hide resolved
@hossain-rayhan
Copy link
Contributor Author

Thanks. I will send another PR with updated README and your few minor suggestions.

@bogdandrutu
Copy link
Member

Not sure I understand, are you going to close this PR and send a new one?

@hossain-rayhan
Copy link
Contributor Author

Not sure I understand, are you going to close this PR and send a new one?

Sorry, my bad. I will send an update (commit).

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Please rebase

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@hossain-rayhan hossain-rayhan changed the title [In Progress] add helper function to convert resource attributes to labels [exporter-helper] add helper function to convert resource attributes to labels Nov 6, 2020
@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu, will appreciate a final look from you.
I rebased the code. Still, the contrib-repo test is failing. I don't think it is happening because any of my changes.

@hossain-rayhan hossain-rayhan changed the title [exporter-helper] add helper function to convert resource attributes to labels [exporter-helper] [resource_to_label_conversion] add helper function and expose exporter settings Nov 6, 2020
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Almost LGTM

if dataPoint.IsNil() {
continue
}
newLabelMap.ForEach(func(k, v string) {
Copy link
Member

Choose a reason for hiding this comment

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

These 3 lines are code duplicate, can have a simple func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, they look similar but are of different types. From my understanding, for creating a different function we need to introduce some kind of interface/wrapping over these types. I think maybe it's OK to have this simple and straight forward solution rather than making it more complex.
I also found some similar patterns in our existing codebase. your call- would appreciate a second thought.

Copy link
Member

Choose a reason for hiding this comment

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

Are they? You have two StringMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind. I got it wrong earlier- I thought about about line 97-100. Thanks, it makes the code nicer. Pushed an update.

exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
)

// ResourceToLabelSettings defines configuration for converting resource attributes to labels
type ResourceToLabelSettings struct {
Copy link
Member

Choose a reason for hiding this comment

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

ResourceToTelemetey? We want to extend in the future to do resource to span attributes and log attributes.

Copy link
Member

Choose a reason for hiding this comment

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

Same changes applies to With func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants