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

Fix metric SetAggregate didn't work #8183

Closed

Conversation

stitchcula
Copy link
Contributor

@stitchcula stitchcula commented Sep 27, 2020

Fixes the bug that metric SetAggregate didn't work, when using sync.Pool to reserve metrics from outputs.

Required for all PRs:

  • Signed CLA.
  • Has appropriate unit tests.

@reimda
Copy link
Contributor

reimda commented Sep 28, 2020

SetAggregate is a deprecated interface. See

// This method may be removed in the future and its use is discouraged.

It's not used in telegraf any more. Can you share more detail why you need it to work?

@stitchcula
Copy link
Contributor Author

SetAggregate is a deprecated interface. See

// This method may be removed in the future and its use is discouraged.

It's not used in telegraf any more. Can you share more detail why you need it to work?

In my fork, I use sync.Pool to reuse metrics, just like:

type reservable struct {
	telegraf.Metric
}

var pool = &sync.Pool{New: func() interface{} {
	m, _ := metric.New("", nil, nil, time.Time{})
	return &reservable{Metric: m}
}

// call it to get blank Metric in parser plugin
func NewMetric() telegraf.Metric {
	return pool.Get().(*reservable)
}

func reserve(m *reservable) {
	m.SetName("")
	m.SetTime(zeroTime)
	m.SetAggregate(false) // it always set aggregate true

	//...

	pool.Put(m)
}

//...

func (m *metric) Accept() {
	reserve(m)
}

func (m *metric) Drop() {
	reserve(m)
}

In function reserve, m.SetAggregate(false) always set aggregate true, because of an obvious mistake in metric.go:

m.aggregate = true

As well, reusing metric can significantly improve the pressure of GC. Maybe I can start a new pull request?

@reimda
Copy link
Contributor

reimda commented Sep 29, 2020

Since SetAggregate() is deprecated and not used anywhere I would rather remove it along with IsAggregate() and metric.aggregate rather than fix it and add tests for it.

@sjwang90 sjwang90 added fix pr to fix corresponding bug wip labels Nov 25, 2020
@srebhan
Copy link
Member

srebhan commented Dec 15, 2020

@stitchcula any progress on this?

@sspaink
Copy link
Contributor

sspaink commented Jul 28, 2021

Closing due to no response, feel free to re-open if you are still interested in completing this pr. Thanks!

@sspaink sspaink closed this Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants