Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

add metric producer manager. #1039

Merged
merged 6 commits into from
Mar 4, 2019
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
1 change: 1 addition & 0 deletions metric/metricexport/producer.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
)

// Producer is a source of metrics.
// Deprecated in lieu of go.opencensus.io.metric.producer.Producer
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply delete it, we don't use this anywhere correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted.

type Producer interface {
// Read should return the current values of all metrics supported by this
// metric provider.
Expand Down
80 changes: 80 additions & 0 deletions metric/producer/manager.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright 2019, OpenCensus Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package producer

import (
"sync"
)

type manager struct {
mu sync.RWMutex
producers []Producer
Copy link
Contributor

@songy23 songy23 Feb 28, 2019

Choose a reason for hiding this comment

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

Consider using a map[string]Producer map[Producer]struct{}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had considered that before but map cannot be unmodifiable. However slices are unmodifiable implicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can implement your own immutable map/slice (also not sure that slices are immutable) by cloning the current map/slice and apply the mutation operation on the clone then swap the pointers to the map/slice.

}

var prodMgr *manager
var once sync.Once

func getManager() *manager {
once.Do(func() {
prodMgr = &manager{}
})
return prodMgr
}
// Add adds the producer to the manager if it is not already present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe define what is the "Manager"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

func Add(producer Producer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to have these function on a "ProducerManager" struct and have a "GetGlobalProducerManager". This way the exporters will get a "ProducerManager" as an option, this is a bit more flexible. We can do that in a separate PR just to make sure we make progress, but I think is a better design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

pm := getManager()
pm.add(producer)
}

// Delete deletes the producer from the manager if it is present.
func Delete(producer Producer) {
pm := getManager()
pm.delete(producer)
}

// GetAll returns all producer registered with the manager. It is typically
// used by exporter to read metrics from producers.
func GetAll() []Producer{
pm := getManager()
return pm.producers
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider returning a copy of producer slice, otherwise this may introduce synchronization issue. In Java we always return an unmodifiable snapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slices are unmodifiable implicitly. I have added a test for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not true:
I think this is a good article to read https://medium.com/@nitishmalhotra/uh-ohs-in-go-slice-of-pointers-c0a30669feee

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave you the wrong link. The idea is that Slice is a struct {len, cap, pointerToData}. If passed by value all of these will get copied but the underlying memory where pointerToData points is shared between multiple copies of the slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it. It is NOT immutable. I verified by a different test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change the implementation.

}

func (pm *manager) contains(producer Producer) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: if contains is only used in add, maybe it's not worth adding a separate method; otherwise contains should be guarded with mu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed contains.

for _, prod := range pm.producers {
if producer == prod {
return true
}
}
return false
}

func (pm *manager) add(producer Producer) {
pm.mu.Lock()
defer pm.mu.Unlock()
if pm.contains(producer) {
return
}
pm.producers = append(pm.producers, producer)
}

func (pm *manager) delete(producer Producer) {
pm.mu.Lock()
defer pm.mu.Unlock()
for index, prod := range pm.producers {
if producer == prod {
pm.producers = append(pm.producers[:index], pm.producers[index+1:]...)
}
}
}
92 changes: 92 additions & 0 deletions metric/producer/manager_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright 2019, OpenCensus Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package producer

import (
"testing"
"go.opencensus.io/metric/metricdata"
)

type testProducer struct {
name string
}

var (
myProd1 = NewTestProducer("foo")
myProd2 = NewTestProducer("bar")
myProd3 = NewTestProducer("foobar")
)

func NewTestProducer(name string) *testProducer {
return &testProducer{name}
}

func (mp *testProducer) Read() []*metricdata.Metric {
return nil
}

func TestAdd(t *testing.T) {
Add(myProd1)
Add(myProd2)

got := GetAll()
want := []*testProducer{myProd1, myProd2}
checkSlice("add test", got, want, t)
}

func TestAddSame(t *testing.T) {
Add(myProd1)
Add(myProd2)
Add(myProd1)

got := GetAll()
want := []*testProducer{myProd1, myProd2}
checkSlice("add test", got, want, t)
}

func TestDelete(t *testing.T) {
Add(myProd1)
Add(myProd2)
Add(myProd3)
Delete(myProd2)

got := GetAll()
want := []*testProducer{myProd1, myProd3}
checkSlice("add test", got, want, t)
}

func TestDeleteNonExisting(t *testing.T) {
Add(myProd1)
Add(myProd3)
Delete(myProd2)

got := GetAll()
want := []*testProducer{myProd1, myProd3}
checkSlice("add test", got, want, t)
}

func checkSlice(testName string, got []Producer, want []*testProducer, t *testing.T) {
gotLen := len(got)
wantLen := len(want)
if gotLen != wantLen {
t.Errorf("test: %s, got len: %d want: %d\n", testName, gotLen, wantLen)
} else {
for i := 0 ; i<gotLen; i++ {
if got[i] != want[i] {
t.Errorf("test: %s, at index %d, got %p, want %p\n", testName, i, got[i], want[i])
}
}
}
}
28 changes: 28 additions & 0 deletions metric/producer/producer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright 2019, OpenCensus Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package producer

import (
"go.opencensus.io/metric/metricdata"
)

// Producer is a source of metrics.
type Producer interface {
// Read should return the current values of all metrics supported by this
// metric provider.
// The returned metrics should be unique for each combination of name and
// resource.
Read() []*metricdata.Metric
}