Skip to content

Conversation

@xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Feb 19, 2025

Closes: #1814

This issue won't cause any major breakdown since it's only for logging the error message.

If the change is valid, then it needs to be updated for all exporters.

@kaylareopelle
Copy link
Contributor

Do we need to update the min version of the protobuf dependencies for the gems too?

@xuan-cao-swi
Copy link
Contributor Author

Do we need to update the min version of the protobuf dependencies for the gems too?

I'm not very familiar with protobuf but I can share what I found:

I don't think we need to update protobuf dependencies (well_known_types.rb file's last commit is 2 years ago); I think that's just an edge case that was missed.

By looking at the well_known_types.rb, it seems like we can access type_name ? But strangely it returns no method when access it.

The type_name seems also derived from type_url (e.g. type_url split), so I think it's safe to use type_url split in our implementation.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2025

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Apr 4, 2025
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Hi @xuan-cao-swi, coming back to this one. I think this is a good change. Would you mind adding a regression test?

@github-actions github-actions bot removed the stale label Apr 5, 2025
@xuan-cao-swi
Copy link
Contributor Author

Hi @kaylareopelle, I think this test cover the case; but I still add one test case that parse the request from raw byte.

I still couldn't figure out why this type_name is missing, so I asked on protobuf repo.

FYI, our current test case use protobuf 3.25, but latest one is 4.30.

@kaylareopelle
Copy link
Contributor

Hi @kaylareopelle, I think this test cover the case; but I still add one test case that parse the request from raw byte.

I still couldn't figure out why this type_name is missing, so I asked on protobuf repo.

FYI, our current test case use protobuf 3.25, but latest one is 4.30.

Thank you and thanks for raising the issue! I think it would be helpful to start testing using newer versions of protobuf. I created a PR to do this, but it fell down in priority and I couldn't see it through to the end. Feel free to open one if you have the bandwidth!

@kaylareopelle kaylareopelle merged commit e590548 into open-telemetry:main Apr 9, 2025
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MetricsReporter: possible outdate protobuf datatype

2 participants