-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[DRAFT] [pdata] Mutable/immutable pdata with interfaces #7088
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
Conversation
94eed2a
to
b5691d3
Compare
b5691d3
to
c0a08e3
Compare
type common${structName} interface { | ||
Len() int | ||
CopyTo(dest Mutable${structName}) | ||
getOrig() *[]*${originName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? I may miss something.
We still need an unexported func to disallow external implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need an unexported func to disallow external implementations.
Why? I thought this should be able to play that role. Another reason for this is to simplify the access to the orig from interface instances passed as arguments, so instead of
func (es immutableSummaryDataPointSlice) CopyTo(dest MutableSummaryDataPointSlice) {
destCap := cap(*dest.(mutableSummaryDataPointSlice).getOrig())
we have
func (es immutableSummaryDataPointSlice) CopyTo(dest MutableSummaryDataPointSlice) {
destCap := cap(*dest.getOrig())
f134edf
to
b7ed117
Compare
b7ed117
to
435a4aa
Compare
// Must use NewSpan function to create new instances. | ||
// Important: zero-initialized instance is not valid for use. | ||
type Span struct { | ||
type Span interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative solution, with less code, no interfaces:
// Copyright The OpenTelemetry 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.
// Code generated by "pdata/internal/cmd/pdatagen/main.go". DO NOT EDIT.
// To regenerate this file run "make genpdata".
package ptrace
import (
"go.opentelemetry.io/collector/pdata/internal"
"go.opentelemetry.io/collector/pdata/internal/data"
otlptrace "go.opentelemetry.io/collector/pdata/internal/data/protogen/trace/v1"
"go.opentelemetry.io/collector/pdata/pcommon"
)
// Span represents a single operation within a trace.
// See Span definition in OTLP: https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto
type Span struct {
commonSpan
}
type MutableSpan struct {
commonSpan
}
type commonSpan struct {
orig *otlptrace.Span
}
func newSpan(orig *otlptrace.Span) Span {
return Span{commonSpan: commonSpan{orig: orig}}
}
func newMutableSpan(orig *otlptrace.Span) MutableSpan {
return MutableSpan{commonSpan: commonSpan{orig: orig}}
}
// NewSpan creates a new empty Span.
//
// This must be used only in testing code. Users should use "AppendEmpty" when part of a Slice,
// OR directly access the member if this is embedded in another struct.
func NewSpan() MutableSpan {
return newMutableSpan(&otlptrace.Span{})
}
// MoveTo moves all properties from the current struct overriding the destination and
// resetting the current instance to its zero value
func (ms MutableSpan) MoveTo(dest MutableSpan) {
*dest.orig = *ms.orig
*ms.orig = otlptrace.Span{}
}
// TraceID returns the traceid associated with this Span.
func (ms commonSpan) TraceID() pcommon.TraceID {
return pcommon.TraceID(ms.orig.TraceId)
}
// SetTraceID replaces the traceid associated with this Span.
func (ms MutableSpan) SetTraceID(v pcommon.TraceID) {
ms.orig.TraceId = data.TraceID(v)
}
// SpanID returns the spanid associated with this Span.
func (ms commonSpan) SpanID() pcommon.SpanID {
return pcommon.SpanID(ms.orig.SpanId)
}
// SetSpanID replaces the spanid associated with this Span.
func (ms MutableSpan) SetSpanID(v pcommon.SpanID) {
ms.orig.SpanId = data.SpanID(v)
}
// TraceState returns the tracestate associated with this Span.
func (ms commonSpan) TraceState() pcommon.TraceState {
return internal.NewImmutableTraceState(&ms.orig.TraceState)
}
// TraceState returns the tracestate associated with this Span.
func (ms MutableSpan) TraceState() pcommon.MutableTraceState {
return internal.NewMutableTraceState(&ms.orig.TraceState)
}
// ParentSpanID returns the parentspanid associated with this Span.
func (ms commonSpan) ParentSpanID() pcommon.SpanID {
return pcommon.SpanID(ms.orig.ParentSpanId)
}
// SetParentSpanID replaces the parentspanid associated with this Span.
func (ms MutableSpan) SetParentSpanID(v pcommon.SpanID) {
ms.orig.ParentSpanId = data.SpanID(v)
}
// Name returns the name associated with this Span.
func (ms commonSpan) Name() string {
return ms.orig.Name
}
// SetName replaces the name associated with this Span.
func (ms MutableSpan) SetName(v string) {
ms.orig.Name = v
}
// Kind returns the kind associated with this Span.
func (ms commonSpan) Kind() SpanKind {
return SpanKind(ms.orig.Kind)
}
// SetKind replaces the kind associated with this Span.
func (ms MutableSpan) SetKind(v SpanKind) {
ms.orig.Kind = otlptrace.Span_SpanKind(v)
}
// StartTimestamp returns the starttimestamp associated with this Span.
func (ms commonSpan) StartTimestamp() pcommon.Timestamp {
return pcommon.Timestamp(ms.orig.StartTimeUnixNano)
}
// SetStartTimestamp replaces the starttimestamp associated with this Span.
func (ms MutableSpan) SetStartTimestamp(v pcommon.Timestamp) {
ms.orig.StartTimeUnixNano = uint64(v)
}
// EndTimestamp returns the endtimestamp associated with this Span.
func (ms commonSpan) EndTimestamp() pcommon.Timestamp {
return pcommon.Timestamp(ms.orig.EndTimeUnixNano)
}
// SetEndTimestamp replaces the endtimestamp associated with this Span.
func (ms MutableSpan) SetEndTimestamp(v pcommon.Timestamp) {
ms.orig.EndTimeUnixNano = uint64(v)
}
// Attributes returns the Attributes associated with this Span.
func (ms commonSpan) Attributes() pcommon.Map {
return internal.NewImmutableMap(&ms.orig.Attributes)
}
func (ms MutableSpan) Attributes() pcommon.MutableMap {
return internal.NewMutableMap(&ms.orig.Attributes)
}
// DroppedAttributesCount returns the droppedattributescount associated with this Span.
func (ms commonSpan) DroppedAttributesCount() uint32 {
return ms.orig.DroppedAttributesCount
}
// SetDroppedAttributesCount replaces the droppedattributescount associated with this Span.
func (ms MutableSpan) SetDroppedAttributesCount(v uint32) {
ms.orig.DroppedAttributesCount = v
}
// Events returns the Events associated with this Span.
func (ms commonSpan) Events() SpanEventSlice {
return newImmutableSpanEventSlice(&ms.orig.Events)
}
func (ms MutableSpan) Events() MutableSpanEventSlice {
return newMutableSpanEventSlice(&ms.orig.Events)
}
// DroppedEventsCount returns the droppedeventscount associated with this Span.
func (ms commonSpan) DroppedEventsCount() uint32 {
return ms.orig.DroppedEventsCount
}
// SetDroppedEventsCount replaces the droppedeventscount associated with this Span.
func (ms MutableSpan) SetDroppedEventsCount(v uint32) {
ms.orig.DroppedEventsCount = v
}
// Links returns the Links associated with this Span.
func (ms commonSpan) Links() SpanLinkSlice {
return newImmutableSpanLinkSlice(&ms.orig.Links)
}
func (ms MutableSpan) Links() MutableSpanLinkSlice {
return newMutableSpanLinkSlice(&ms.orig.Links)
}
// DroppedLinksCount returns the droppedlinkscount associated with this Span.
func (ms commonSpan) DroppedLinksCount() uint32 {
return ms.orig.DroppedLinksCount
}
// SetDroppedLinksCount replaces the droppedlinkscount associated with this Span.
func (ms MutableSpan) SetDroppedLinksCount(v uint32) {
ms.orig.DroppedLinksCount = v
}
// Status returns the status associated with this Span.
func (ms commonSpan) Status() Status {
return newImmutableStatus(&ms.orig.Status)
}
// Status returns the status associated with this MutableSpan.
func (ms MutableSpan) Status() MutableStatus {
return newMutableStatus(&ms.orig.Status)
}
// CopyTo copies all properties from the current struct overriding the destination.
func (ms commonSpan) CopyTo(dest MutableSpan) {
dest.SetTraceID(ms.TraceID())
dest.SetSpanID(ms.SpanID())
ms.TraceState().CopyTo(dest.TraceState())
dest.SetParentSpanID(ms.ParentSpanID())
dest.SetName(ms.Name())
dest.SetKind(ms.Kind())
dest.SetStartTimestamp(ms.StartTimestamp())
dest.SetEndTimestamp(ms.EndTimestamp())
ms.Attributes().CopyTo(dest.Attributes())
dest.SetDroppedAttributesCount(ms.DroppedAttributesCount())
ms.Events().CopyTo(dest.Events())
dest.SetDroppedEventsCount(ms.DroppedEventsCount())
ms.Links().CopyTo(dest.Links())
dest.SetDroppedLinksCount(ms.DroppedLinksCount())
ms.Status().CopyTo(dest.Status())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I was trying to avoid the common interface but it appeared to be required for most of the cases because of method name conflicts. So this approach should be less code indeed. I'll give it a try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdandrutu looks like MutableSpan and Span are still convertible between each other unless we add another private field to MutableSpan to prevent Span->MutableSpan conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this PR for now, and submitted another draft instead #7140
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closing in favor of #7140 |
This is a PR alternative to #6798. The goal is the same: a compile-time solution to #6794.
Instead of introducing mutable structures with duplicated immutable methods, this PR utilizes interfaces to reduce the duplication by "embedding" non-mutable methods in the mutable interfaces.
The downside of this approach is that we have to bring back the aliasing for
pcommon
package since I don't see a way to expose an interface and have its implementation parts in theinternal
package.The PR doesn't touch tests yet.