propono icon indicating copy to clipboard operation
propono copied to clipboard

Need a way to pass Subject for the SNS payload

Open MythosRaconteur opened this issue 8 years ago • 9 comments

Hey Jeremy,

Apologies if this is the wrong place to put this, but I wasn't sure where to make the request.

I am using Propono to publish SNS messages, and then consuming them with Lambdas that integrate with various 3rd party services (like Slack, and many others).

Our topics are notifiable elements (essentially business objects), with one Lambda acting as a message broker, figuring out which integration to hit, via resubmission of a SNS package against a modified topic. The modified topic is the original topic plus the integration (i.e. user-slack).

The "Slack" Lambda is triggered from that message and fires of a message to Slack.

My issue is that I need the original path accessible to the broker Lambda, and while I could parse the ARN, it would be much easier to just use the Subject of the SNS package, which seems to always be nil with Propono-generated messages.

I altered the services/publisher.rb script by passing options to the sns.publish method:

def publish_via_sns_syncronously
      begin
        topic = TopicCreator.find_or_create(topic_id)
      rescue => e
        Propono.config.logger.error "Propono [#{id}]: Failed to create topic #{topic_id}: #{e}"
        raise
      end

      begin
        sns.publish(topic.arn, body.to_json, {'Subject' => topic_id})
      rescue => e
        Propono.config.logger.error "Propono [#{id}]: Failed to send via sns: #{e}"
        raise
      end
end

While this works, it would, obviously, be better if it were actually parameterized so I could pass it in with the call to Propono.publish and not have to mod your code.

Is there any issue you see with this? If not, I can put it together in a pull and send it up.

Cheers,

Chris

MythosRaconteur avatar Jul 20 '17 01:07 MythosRaconteur

Hey @MythosRaconteur - thanks for posting. Let me have a think about this over the weekend and try and work something out. Thanks.

iHiD avatar Jul 21 '17 10:07 iHiD

Hey there, I actually pulled my change out of your code and decided to parse the TopicArn for the value. It works fine, but it would be nice if there was a raw value on the JSON object, however that is affected.

Cheers,

C

MythosRaconteur avatar Jul 21 '17 17:07 MythosRaconteur

Hi Guys

What if any "subject" from the payload hash was used as the SNS topic subject?

    Propono.publish(
      'user',
      {
        subject: 'my subject'
        entity: 'user',
        action: 'created',
        id: user.id,
      }
    )

I'm thinking that if we add a parameter to publish and use that as the SNS topic subject, it wouldn't be available when reading the message off the SQS queue. That would seem odd to me if I wasn't using lambdas. But having it in the payload there is nothing lost when using pub/sub via the SQS queues.

Great suggestion @MythosRaconteur :-)

Malc

malcyL avatar Jul 22 '17 06:07 malcyL

@malcyL Yeah - I like that idea. My only concern is if subject already has meaning for someone in the payload. Are there rules around what the SNS topic subject can be? If so, the two might clash causing a breaking change (e.g. if SNS-topic-subject has to be ascii and someone is using unicode in their payload).

We could do both, so you can specify subject on publish. If you don't then it uses the subject from the payload. If you want to stop that later behaviour, you could set subject to nil. We could also make it a config option in general?

Thoughts?

iHiD avatar Jul 22 '17 19:07 iHiD

@iHiD I wonder if both are overkill. I was trying to make it simpler, but two mechanisms and a flag are making it more complicated.

Maybe the subject parameter on publish is the way to go.

I was trying to think of a way for the subject to still be available when you are consuming the messages via Propono.listen_to_queue, but maybe that's not important? From just an API perspective, ignoring how it's implemented, we will have a subject added to publish which is then never exposed when reading. Unless your using lambdas of course!

But I don't want to over complicate it. An optional parameter on publish is more desirable than two mechanisms in my opinion.

malcyL avatar Jul 23 '17 07:07 malcyL

Did mean to open a can of worms. ;)

At the end of the day, it is simple enough to split the TopicArn and pull the topic off the end, but it would be convenient if you could just get it directly from its own key.

Is "Subject" on the SNS payload ever used, and if so, what is it intended to be?

Barring the use of that key, perhaps the answer is to just inject "Topic" at the same level?

MythosRaconteur avatar Jul 23 '17 14:07 MythosRaconteur

@mythosRaconteur No can of worms - discussion is always good. 🙂

malcyL avatar Jul 23 '17 14:07 malcyL

Hello. v2 is now released and I'm in Propono mindset. Do I need to do anything here? :)

iHiD avatar Sep 29 '17 12:09 iHiD

I worked around it. Might be nice to expose it more easily, but does not seem to be much need beyond my own, so I would be fine either way.

Cheers,

C

On Sep 29, 2017, at 5:00 AM, Jeremy Walker [email protected] wrote:

Hello. v2 is now released and I'm in Propono mindset. Do I need to do anything here? :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/iHiD/propono/issues/72#issuecomment-333107796, or mute the thread https://github.com/notifications/unsubscribe-auth/AAi5ExBNZuTmeuGwrs1yiUmbWUT9nppIks5snNvfgaJpZM4Odes_.

MythosRaconteur avatar Sep 29 '17 14:09 MythosRaconteur