Skip to content

Commit

Permalink
remove separate name field, single instrument validation
Browse files Browse the repository at this point in the history
  • Loading branch information
Kristina Pathak committed Aug 3, 2023
1 parent 494ec83 commit 764e1c6
Show file tree
Hide file tree
Showing 5 changed files with 6 additions and 60 deletions.
3 changes: 0 additions & 3 deletions lightstep/sdk/metric/internal/viewstate/viewstate.go
Original file line number Diff line number Diff line change
Expand Up @@ -694,9 +694,6 @@ func viewDescriptor(instrument sdkinstrument.Descriptor, v view.ClauseConfig) sd
name := v.Rename(instrument.Name)
description := instrument.Description
unit := instrument.Unit
if v.HasName() {
name = v.Name()
}
if v.Description() != "" {
description = v.Description()
}
Expand Down
15 changes: 0 additions & 15 deletions lightstep/sdk/metric/internal/viewstate/viewstate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1446,21 +1446,6 @@ func TestRegexpMatch(t *testing.T) {
require.NotNil(t, inst2)
}

func TestSingleInstrumentWarning(t *testing.T) {
views := view.New(
"test",
safePerf,
view.WithClause(
view.MatchInstrumentNameRegexp(regexp.MustCompile(".*_rate")),
view.WithName("fixed"),
),
)

_, err := view.Validate(views)
require.Error(t, err)
require.Contains(t, err.Error(), "multi-instrument view specifies a single name")
}

func TestDeltaTemporalityMinMaxSumCount(t *testing.T) {
views := view.New(
"test",
Expand Down
24 changes: 5 additions & 19 deletions lightstep/sdk/metric/view/clause.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ type ClauseConfig struct {

// Properties of the view
keys []attribute.Key // nil implies all keys, []attribute.Key{} implies none
name string
renameFunc RenameInstrumentFunction
description string
aggregation aggregation.Kind
Expand Down Expand Up @@ -112,14 +111,13 @@ func WithKeys(keys []attribute.Key) ClauseOption {
}

func WithName(name string) ClauseOption {
return clauseOptionFunction(func(clause ClauseConfig) ClauseConfig {
clause.name = name
return clause
return WithRenameFunction(func(_ string) string {
return name
})
}

// WithRenameFunction provides a function for renaming chosen instruments. This
// should not be set with WithName().
// should not be set with WithName() - whichever is applied last will be used.
func WithRenameFunction(renameFunc RenameInstrumentFunction) ClauseOption {
return clauseOptionFunction(func(clause ClauseConfig) ClauseConfig {
clause.renameFunc = renameFunc
Expand Down Expand Up @@ -148,20 +146,8 @@ func WithAggregatorConfig(acfg aggregator.Config) ClauseOption {
})
}

// IsSingleInstrument is a requirement when HasName().
func (c *ClauseConfig) IsSingleInstrument() bool {
return c.instrumentName != ""
}

// HasName implies IsSingleInstrument SHOULD be required.
func (c *ClauseConfig) HasName() bool {
return c.name != ""
}

func (c *ClauseConfig) Name() string {
return c.name
}

// Rename executes the rename function on the name provided. If no rename
// function was set, the original name is returned.
func (c *ClauseConfig) Rename(name string) string {
if c.renameFunc == nil {
return name
Expand Down
11 changes: 0 additions & 11 deletions lightstep/sdk/metric/view/views.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,6 @@ func Validate(v *Views) (*Views, error) {
for i := range valid.Clauses {
clause := &valid.Clauses[i]

if !clause.IsSingleInstrument() && clause.HasName() {
// Note: no correction, this condition creates conflicts.
err = multierr.Append(err, fmt.Errorf("multi-instrument view specifies a single name"))
}

err = v.checkAggregation(err, &clause.aggregation, aggregation.UndefinedKind)
err = v.checkAggConfig(err, &clause.acfg)

Expand All @@ -120,12 +115,6 @@ func Validate(v *Views) (*Views, error) {
clause.instrumentNameRegexp = nil
}

if clause.name != "" && clause.renameFunc != nil {
err = multierr.Append(err, fmt.Errorf("view has name and rename function"))
// Note: prefer the name over the function.
clause.renameFunc = nil
}

for i := range clause.keys {
if clause.keys[i] == "" {
err = multierr.Append(err, fmt.Errorf("view has empty string in keys"))
Expand Down
13 changes: 1 addition & 12 deletions lightstep/sdk/metric/view/views_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func TestClauseProperties(t *testing.T) {

views, err := Validate(views)
require.NoError(t, err)
require.Equal(t, "longname", views.Clauses[0].Name())
require.Equal(t, "longname", views.Clauses[0].Rename(""))
require.Equal(t, "very interesting", views.Clauses[1].Description())
require.Equal(t, []attribute.Key(nil), views.Clauses[2].Keys())
require.Equal(t, []attribute.Key{}, views.Clauses[3].Keys())
Expand Down Expand Up @@ -155,17 +155,6 @@ func TestEmptyKeyString(t *testing.T) {
require.Contains(t, err.Error(), "view has empty string in keys")
}

func TestSingleNameConflict(t *testing.T) {
views := New("test", safePerf, WithClause(
WithName("aha"),
))

_, err := Validate(views)

require.Error(t, err)
require.Contains(t, err.Error(), "multi-instrument view specifies a single name")
}

func TestStandardTemporality(t *testing.T) {
views := New("test", safePerf,
WithDefaultAggregationTemporalitySelector(StandardTemporality),
Expand Down

0 comments on commit 764e1c6

Please sign in to comment.