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.

Sign up for GitHub

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

Jump to bottom

Attributes の再設計 #10

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Attributes の再設計 #10

merged 2 commits into from
Jan 16, 2024

Conversation

kakkun61
Copy link
Member

変更：

  • Attributes → AttributeCollection
    • ドロップ機能付きのマップ
  • HashMap Text Attribute → Attributes
    • ただのマップ
  • Text → Key a
    • キーを単なるテキストから、バリューの型付きのキー型に変更
    • lookup したときのパターンマッチをなくす

@kakkun61
rearrange attributes
bdb7231 
Change:
Attributes → AttributeCollection
HashMap Text Attribute → Attributes

New: Key for Attributes
@kakkun61 kakkun61 self-assigned this Dec 21, 2023
@kakkun61
Copy link
Member Author

結構大きなインターフェースの変更だけど、本家に受け入れてもらえるかなあ

@ynishinaka インターフェースを変えようと思いますというお知らせと、変更に関する意見があればほしいです

@ynishinaka
Copy link

ynishinaka commented Dec 21, 2023 •
edited
Loading

とりあえず、このPRのモチベが分かると嬉しいなと思いました。あとドロップ機能の説明も欲しいです

@kakkun61
Copy link
Member Author

ドロップ機能は AttributeLimits で定義される count を超える数の attribute が挿入されない、length を超える長さの attribute が挿入されない機能です

https://opentelemetry.io/docs/specs/otel/common/#attribute-limits

@kakkun61
Copy link
Member Author

動機としては

  • lookup でパターンマッチが不要になるように Key a にしたい
  • そうすると、HashMap _ _ に名前を付ける必要がある＆操作するための関数が必要
  • 名前は、かなり悩んだが Attributes の名前を HashMap _ _ にゆずり、元々の Attributes を AttributeCollection に
    • AttributeCollection の名前はここから取った
  • 操作するための関数は Attributes と AttributeCollection で似通うので名前空間を分けた

です

@kakkun61
Copy link
Member Author

@ynishinaka 22:30 以降ならいけます

@ynishinaka
Copy link

ynishinaka commented Dec 22, 2023 •
edited
Loading

@kakkun61 説明ありがとうございます！目的は分かったんですが、その目的に対して少しAPI変更のコストが重すぎる感じがします。来週以降でもうちょっと相談していきたいです 🙏

@ynishinaka
Copy link

22:30 以降ならいけます

すみません一回聞いておいてアレなんですが、今日はもう遅くなってきたので来週以降また相談させてください！

@kakkun61
Copy link
Member Author

はーい

@kakkun61 kakkun61 force-pushed the attributes branch 4 times, most recently from 8922fa7 to f0cfaa7 Compare January 12, 2024 05:29
@kakkun61
Copy link
Member Author

@ynishinaka 再レビューお願いします

@ynishinaka
Copy link

ynishinaka commented Jan 12, 2024 •
edited
Loading

実装ありがとうございます！！雰囲気は大体よさそうな感じがするので、!パターンまわりだけ確認おねがいします 🙇

@kakkun61
HashMap Text Attribute の newtype をつくるのをやめる
47310b5 
IsAttribute を ToAttribute と FromAttribute に分ける
モジュール名変える
ynishinaka
ynishinaka approved these changes Jan 16, 2024
View reviewed changes
Copy link

@ynishinaka ynishinaka left a comment

Choose a reason for hiding this comment

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

割とよさそうです！ありがとうございます！！！

@kakkun61 kakkun61 merged commit 3c911f7 into main Jan 16, 2024
10 checks passed
@kakkun61 kakkun61 deleted the attributes branch January 16, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ynishinaka ynishinaka ynishinaka approved these changes

Assignees

@kakkun61 kakkun61

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kakkun61 @ynishinaka