Fix Graphite Tag integration by sarthaksin1857 · Pull Request #131 · apache/otava

sarthaksin1857 · 2026-02-22T19:02:30Z

What

Change example image to alpine and install bash/curl (datagen was not working when curl was uncommented)
Make all fields optional with default values in GraphiteEvent

Proof it works

With the graphite example in the repo, we now see

time                       run    branch       version    commit      throughput    response_time    cpu_usage
-------------------------  -----  -----------  ---------  --------  ------------  ---------------  -----------
2026-02-24 04:29:10 +0000         new-feature  0.0.1      p7q8r9           61160               87          0.2
2026-02-24 04:30:10 +0000         new-feature  0.0.1      m4n5o6           60160               85          0.3
2026-02-24 04:31:10 +0000         new-feature  0.0.1      j1k2l3           60960               89          0.1
                                                                    ············                   ···········  
                                                                           -5.6%                       +300.0%  
                                                                    ············                   ···········  
2026-02-24 04:32:10 +0000         new-feature  0.0.1      g7h8i9           57123               88          0.8
2026-02-24 04:33:10 +0000         new-feature  0.0.1      d4e5f6           57980               87          0.9
2026-02-24 04:34:10 +0000         new-feature  0.0.1      a1b2c3           56950               85          0.7

Which makes sense as we do not set run but we do set everything else

Fix data generation script to send required fields nit remove non graphite changes nit Add datagen changes Fails still but can deploy local code Set defaults for all variables and manually specify pub_time Add table on supported metadata field nit Only confirm pub_time is set properly

sarthaksin1857 · 2026-02-24T04:41:08Z

@Gerrrr Can you take a look here

Gerrrr

Thanks for opening this PR, @sarthaksin1857 ! I found a few things we need to fix before it is ready to merge. Also, given the wealth of subtle issues, this class may deserve a few unit tests.

Gerrrr · 2026-02-27T03:04:30Z

 filter data by commit or version using `--since-commit` or `--since-version` selectors.

+#### Supported Metadata Schema
+The following keys are supported within the `data` dictionary:


It may not be clear what is the data dictionary in this case. In the paragraph above, we talk about tags, so I suggest replacing this statement with The following tags are supported:

Gerrrr · 2026-02-27T03:09:37Z

-    branch: Optional[str]
-    commit: Optional[str]
-
-    def __init__(


there is something funky going on with indentation. Intelij highlights edits in this commit differently from the rest of the file:

Think I fixed it, how does it look now?

Gerrrr · 2026-02-27T03:13:25Z

+        if isinstance(self.pub_time, str):
+            self.pub_time = parse_datetime(self.pub_time)
+        elif isinstance(self.pub_time, (int, float)):
+            self.pub_time = datetime.fromtimestamp(self.pub_time)


This change uses a different parsing logic than parse_datetime. The latter does dateparser.parse(date, settings={"RETURN_AS_TIMEZONE_AWARE": True}).

Why do need these extra manipulations with pub_time? I don't follow how making fields optional required it.

This was more just me looking at this code an how to future proof it.

The function says that pub_time is already a date time and if its already a datetime, why try to parse it again?

Similarly if someone passes in a unix int like 1776522767 the string function wont work either

So before we passed in an integer string as "datetime" and then parsed it back into a string, and then into the proper object.

I will just move the logic into parse_datetime and make this code cleaner

Gerrrr · 2026-02-27T03:14:54Z

-            self.version = None
-        else:
-            self.version = version
-        if len(branch) == 0 or branch == "null":


This logic was lost in this PR. If a string text is empty or equal to "null", we still want to treat it as empty, unless there is a reason not to.

Since I switched all actual "optional" fields to be optional, I will clean all of them (except pub_time)

Gerrrr · 2026-02-27T03:17:23Z

-        end_time: int,
-        version: Optional[str],
-        branch: Optional[str],
-        commit: Optional[str],


idea (maybe outside of scope of this PR): today, if we have other event tags like "environment": "prod", we'll crash with unexpected keyword argument. Maybe we can change this class such that it'd work arbitrary tags.

Yea I wonder if we should even define fields here as "supported tags" and rather just store them in a map and print them all at the end as columns

documentation fixup Address comments Add coverage for parsing Clean all string Indentation nit formatter Auto formatter

sarthaksin1857 · 2026-04-18T16:05:57Z

@Gerrrr sorry for the wait this is ready for another pass.

I made it so

All optional fields are cleaned
the parse_datetime utility handles string, int, long, and datetime inputs
Updated the documentation
Added a few tests

I tried to extend the integration tests but it didn't seem to play well with tags. Whenever I tried sending an event, it would time out (I can file an issue for adding integration coverage and tackle that next?)

sarthaksin1857 force-pushed the fix-graphite-datagen branch from f89d7e2 to 8f70419 Compare February 22, 2026 19:11

sarthaksin1857 force-pushed the fix-graphite-datagen branch from ec4a46e to adc9f02 Compare February 24, 2026 04:38

sarthaksin1857 marked this pull request as ready for review February 24, 2026 04:40

Gerrrr requested changes Feb 27, 2026

View reviewed changes

sarthaksin1857 force-pushed the fix-graphite-datagen branch from a76be64 to 2b019bf Compare April 18, 2026 15:45

make defaults none

19e758c

documentation fixup Address comments Add coverage for parsing Clean all string Indentation nit formatter Auto formatter

sarthaksin1857 force-pushed the fix-graphite-datagen branch from c0ea484 to 19e758c Compare April 18, 2026 15:56

indentation stuff

bfb7c01

Fix my own tests

f3b786a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Graphite Tag integration#131

Fix Graphite Tag integration#131
sarthaksin1857 wants to merge 4 commits intoapache:masterfrom
sarthaksin1857:fix-graphite-datagen

sarthaksin1857 commented Feb 22, 2026 •

edited

Loading

Uh oh!

sarthaksin1857 commented Feb 24, 2026

Uh oh!

Gerrrr left a comment

Uh oh!

Gerrrr Feb 27, 2026

Uh oh!

Gerrrr Feb 27, 2026

Uh oh!

sarthaksin1857 Apr 18, 2026

Uh oh!

Gerrrr Feb 27, 2026

Uh oh!

sarthaksin1857 Apr 18, 2026

Uh oh!

Gerrrr Feb 27, 2026

Uh oh!

sarthaksin1857 Apr 18, 2026

Uh oh!

Gerrrr Feb 27, 2026

Uh oh!

sarthaksin1857 Apr 18, 2026

Uh oh!

sarthaksin1857 commented Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sarthaksin1857 commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Proof it works

Uh oh!

sarthaksin1857 commented Feb 24, 2026

Uh oh!

Gerrrr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarthaksin1857 commented Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sarthaksin1857 commented Feb 22, 2026 •

edited

Loading