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

wip: convert to vtproto #6407

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Jun 1, 2023

WIP: get rid of gogoproto and introduce vtproto

state:

  • converting to golang proto v2 api
  • pkg/store tests are passing
  • pkg/receive tests are passing
  • pkg/targets tests are passing
  • pkg/rules tests are passing
  • pkg/metadata tests are passing

pkg/store/prometheus.go Outdated Show resolved Hide resolved
@@ -306,51 +296,10 @@ func testInjectExtLabels(tb testutil.TB) {
}

var (
zdest ZLabel
zdest *Label
Copy link

Choose a reason for hiding this comment

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

30% of developers fix this issue

U1000: var zdest is unused


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -306,51 +296,10 @@ func testInjectExtLabels(tb testutil.TB) {
}

var (
zdest ZLabel
zdest *Label
dest Label
Copy link

Choose a reason for hiding this comment

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

30% of developers fix this issue

U1000: var dest is unused


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

if len(m.BlockMatchers) > 0 {
for iNdEx := len(m.BlockMatchers) - 1; iNdEx >= 0; iNdEx-- {
{
size, err := m.BlockMatchers[iNdEx].MarshalToSizedBuffer(dAtA[:i])
Copy link

Choose a reason for hiding this comment

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

27% of developers fix this issue

typecheck: m.BlockMatchers[iNdEx].MarshalToSizedBuffer undefined (type *storepb.LabelMatcher has no field or method MarshalToSizedBuffer)

❗❗ 8 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 544
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 626
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 700
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 752
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 788
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 881
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 1134
pkg/github.com/thanos-io/thanos/pkg/store/hintspb/hints.pb.go 1304

Visit the Lift Web Console to find more details in your report.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -306,51 +296,10 @@ func testInjectExtLabels(tb testutil.TB) {
}

var (
zdest ZLabel
zdest *Label
Copy link

Choose a reason for hiding this comment

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

31% of developers fix this issue

unused: var zdest is unused


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -306,51 +296,10 @@ func testInjectExtLabels(tb testutil.TB) {
}

var (
zdest ZLabel
zdest *Label
dest Label
Copy link

Choose a reason for hiding this comment

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

31% of developers fix this issue

unused: var dest is unused


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

pkg/store/prometheus.go Outdated Show resolved Hide resolved
pkg/store/proxy_test.go Outdated Show resolved Hide resolved
func ZLabelsFromPromLabels(lset labels.Labels) []ZLabel {
return *(*[]ZLabel)(unsafe.Pointer(&lset))
// ProtobufLabelsFromPromLabels converts Prometheus labels to a slice pointers to labelpb.Label.
func ProtobufLabelsFromPromLabels(lset labels.Labels) []*Label {
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda defeats the purpose of ZLabels. Can we use the protobuf definitions from Prometheus directly?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can since Prometheus protobuf is generated with gogoprotobuf and that interferes with our code. Worth testing out but I think we might get a similar panic to this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isnt ZLabel going away with prometheus stringlabels anyway?

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-move-to-vt-protobuf branch 2 times, most recently from e7683b5 to c5e0a2e Compare June 3, 2023 12:54
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-move-to-vt-protobuf branch 2 times, most recently from 7eafa8f to c878456 Compare June 15, 2023 15:29
@@ -14,7 +14,7 @@ import (
"sync"
"time"

"github.com/gogo/protobuf/proto"
"github.com/golang/protobuf/proto"
Copy link

Choose a reason for hiding this comment

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

7% of developers fix this issue

SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -10,14 +10,15 @@ import (
"testing"
"time"

"github.com/gogo/protobuf/proto"
"github.com/golang/protobuf/proto"
Copy link

Choose a reason for hiding this comment

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

7% of developers fix this issue

SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -14,7 +14,7 @@ import (
"sync"
"time"

"github.com/gogo/protobuf/proto"
"github.com/golang/protobuf/proto"
Copy link

Choose a reason for hiding this comment

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

staticcheck: SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -10,14 +10,15 @@ import (
"testing"
"time"

"github.com/gogo/protobuf/proto"
"github.com/golang/protobuf/proto"
Copy link

Choose a reason for hiding this comment

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

staticcheck: SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

*: remove and ban github.com/golang/protobuf/proto

Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants